tobac icon indicating copy to clipboard operation
tobac copied to clipboard

Useless field_in in linking_trackpy

Open zxdawn opened this issue 2 years ago • 3 comments

It seems the field_in isn't used in linking_trackpy which only uses the features to link data

https://github.com/tobac-project/tobac/blob/4b9c79857dc9ad17692c6919c44fd02fcbf83cd5/tobac/themes/tobac_v1/tracking.py#L27-L29

Is it better to delete it?

zxdawn avatar Apr 28 '22 14:04 zxdawn

We had a discussion on this in #115. I think the conclusion there was that because this is a positional argument, depreciating it without breaking code is quite challenging. There's also an argument to keep it in if we want to add additional trackpy linking methods that may require the input field, but that doesn't seem very likely.

If we want to depreciate the parameter (which I'm not opposed to), we'd probably have to:

  1. Change the call signature of linking_trackpy to be (*args, **kwargs)
  2. Check the number of positional args, if it includes field_in throw a DepreciationWarning
  3. Eventually phase out the DepreciationWarning and instead check the types of the input arguments to make sure that field_in is still not passed in; if it is throw an error

freemansw1 avatar Apr 29 '22 16:04 freemansw1

We can also resolve this by making a new function (without field_in) and depreciating the old linking_trackpy. That would probably be cleaner from our end, but user code would need to be updated. We'd also have to come up with a new function name (which I am not opposed to).

freemansw1 avatar May 12 '22 14:05 freemansw1

Based on discussion at the developers' meeting today, the first option (changing the call signature) seems to be preferred, but given that the second option is very easy to do, I will make PRs for both and we can decide, knowing that we are leaning toward the first option.

freemansw1 avatar May 13 '22 14:05 freemansw1