search

tobac-project / tobac

Tracking and object-based analysis of clouds
BSD 3-Clause "New" or "Revised" License
98 stars 53 forks source link

Useless field_in in linking_trackpy #125

Open zxdawn opened 2 years ago

zxdawn commented 2 years ago

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?

freemansw1 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.