-
Notifications
You must be signed in to change notification settings - Fork 54
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
Resolves #212 #223
Resolves #212 #223
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, @lettlini! No objections to merging the code changes as they are. Thank you for fixing this one
Codecov ReportBase: 41.01% // Head: 41.06% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## RC_v1.5.0 #223 +/- ##
=============================================
+ Coverage 41.01% 41.06% +0.05%
=============================================
Files 14 14
Lines 2321 2323 +2
=============================================
+ Hits 952 954 +2
Misses 1369 1369
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -178,6 +178,11 @@ def linking_trackpy( | |||
# from trackpy import filter_stubs | |||
# from .utils import add_coordinates | |||
|
|||
if (v_max is None) and (d_min is None) and (d_max is None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I look again at the explanation of d_min
in the docstring, I think we should exclude it here, since its original purpose is not to define a search range (The current implementation, however, only mimics d_max
).
Since the parameter will eventually be deprecated, it is probably better not to promote its use in an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your thoughts here, Nils. I'll wait to review until this is changed. I'd say that we should leaved_min
in the if statement here, but remove it in the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are both absolutely right. I have updated the error message accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this PR. Once @snilsn has a chance to review again, I'm happy for this to be merged.
@lettlini since Nils has approved, I'm happy to merge this; are you still happy for it to be included? |
@freemansw1 Sorry for the late reply. Yes I am still happy for this PR to be merged |
This pull request should resolve #212.
A simple check has been added to correctly handle the case of neither
v_max, d_min, d_max
being passed tolinking_trackpy
, i.e. aValueError
is raised if all three arguments are missing. A corresponding test case has also been added.