Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Fix prediction bugs #710

Merged
merged 12 commits into from
Nov 14, 2022
Merged

FIX: Fix prediction bugs #710

merged 12 commits into from
Nov 14, 2022

Conversation

nkeim
Copy link
Contributor

@nkeim nkeim commented Sep 3, 2022

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:

  • Update the prediction tutorial

Suggested future enhancements:

  • Provide wrapper functions to make it easier to use find_link with a predictor.
  • Accept initial velocity guesses as DataFrames, eliminating the need to specify pos_columns.

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 nkeim mentioned this pull request Sep 3, 2022
@nkeim
Copy link
Contributor Author

nkeim commented Sep 18, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

nkeim commented Nov 14, 2022

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.

@nkeim nkeim merged commit 951c432 into soft-matter:master Nov 14, 2022
@nkeim nkeim modified the milestones: v0.6, 0.6 Dec 23, 2022
@nkeim nkeim mentioned this pull request Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nearestvelocity
2 participants