search

soft-matter / trackpy

Python particle tracking toolkit
http://soft-matter.github.io/trackpy
Other
441 stars 131 forks source link

FIX: Fix prediction bugs #710

Closed nkeim closed 1 year ago

nkeim commented 1 year ago

This PR closes #699 . It tests for and (hopefully) fixes both the link_df issue and the coordinate switching issue. It makes the order of pos_columns used by the linker match the order used by the predictor, whereas before trackpy tried to keep them in reverse order (for backward compatibility, perhaps?). The PR includes some safeguards (tested) against prediction with ambiguous or conflicting pos_columns.

There is (unfortunately) a breaking API change when supplying an initial velocity guess to a predictor. Since these guesses are supplied as arrays (not DataFrames), the order of the coordinates has always been ambiguous. In recent versions including v0.5.1, the order has implicitly been the reverse of the order in the subsequent linking step, so that the columns in the velocity guess typically corresponded to e.g. ['x', 'y', 'z']. I hope you'll agree that this is confusing at best. With this new PR, the predictor uses the same pos_columns as the linker, which is typically e.g. ['z', 'y', 'x'].

Because of this change, NearestVelocityPredict and DriftPredict now require pos_columns to be specified along with any initial velocity guess. There is additional logic to make sure this is the same pos_columns subsequently used by the linking step (the user doesn't need to specify pos_columns a second time). Requiring pos_columns is good for users who are currently supplying initial velocity guesses, since their code will stop working until they resolve this ambiguity, instead of trackpy silently misinterpreting their velocity guess.

Finally, prediction when using find_link() etc. is still barely supported. Even though we test it extensively, using it requires a few things to be done manually that would ordinarily be handled automatically by the predictor instance. The best "documentation" is the code near the end of test_predict.py, which is not a good situation.

Still to do in this PR:

Suggested future enhancements:

I think this API change suggests that we should include these fixes in v0.6. That is inconvenient, but all of the prediction features were usable with enough luck and/or workarounds, and the more correct handling of pos_columns will surely break some existing scripts. I have drafted release notes accordingly.

nkeim commented 1 year ago

I'd love for another maintainer to look this over before merging, since it's an API change (and a lot more changes behind the scenes). @caspervdw since you were the only other person to work on test_predict.py, do you have a little time these days to browse this PR and merge? We've got confirmation from @snilsn that this fixes the bugs in #699.

freemansw1 commented 1 year ago

Just want to note that this fix also allows for predictive tracking with 3D as the workaround that @snilsn proposed in #699 does not work in 3D as far as I can tell.

nkeim commented 1 year ago

Thanks for the confirmation, @freemansw1 !

It seems like this PR has gotten some use over the last 2 months, and considering that it fixes a bug, I will go ahead and merge.