tobac-project / tobac

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

`min_distance` doesn't work with 3D tracking where `dz` not constant #430

Closed freemansw1 closed 1 month ago

freemansw1 commented 6 months ago

Reported to me by Yishi Hu from OU. When you use min_distance in feature detection with a non-constant dz, it fails as the coordinate interpolation takes place after min_distance is called. That means min_distance doesn't have any vertical information. This should be able to be solved by reversing the order of the two calls.

freemansw1 commented 6 months ago

There's a secondary bug in filter_min_distance, line 1534 of main, where it always uses dz even if none is passed.

w-k-jones commented 6 months ago

I realised this when you pointed out the issue with dz in #372 . It's a bit trickier with feature detection, as we will need to perform coordinate interpolation before distance filtering. It may have to wait until I have time to fix it in a later release

freemansw1 commented 6 months ago

I realised this when you pointed out the issue with dz in #372 . It's a bit trickier with feature detection, as we will need to perform coordinate interpolation before distance filtering. It may have to wait until I have time to fix it in a later release

Shouldn't we be able to just do interpolation first without too much of an issue? There's probably a performance penalty there, but I assume it isn't too bad of one.

w-k-jones commented 6 months ago

It probably should be fine, I just haven't had any time recently to look into implementing it. I'll look into making the kdTree/BallTree neighbour search a more generic method to avoid these issues in future, as it it will be useful for other methods

w-k-jones commented 1 month ago

Aha the feature detection code is making what should be a simple fix headache inducing 🙃 I'll see if I can get this fix finished in time for the 1.5.4 release, but it may have to wait until afterwards. It may also conflict with some of the changes in v1.6, so we'll have to watch out for that

w-k-jones commented 1 month ago

Resolved by #452