-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Clean + comment MaxNLocator #25166
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
Clean + comment MaxNLocator #25166
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.
Subject to re-adding the comment. It is quite important that dv > 0 since it is used as a denominator and log is computed on it. So while it will still be > 0, I think it is a worthwhile comment for later reference.
(I might consider micro-optimizing the code to change if abs(meanv) / dv < threshold:
to if abs(meanv) < threshold * dv:
though, but that is just me...
And possibly add a comment that // 1
is floor. Had to think a bit there...)
3657fb9
to
625602a
Compare
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 seems OK, but the code is still actually quite confusing to me. I am not actually sure whether the filtering of large_steps actually matters, or if it is just a small optimization for the for step in steps...
loop below? i.e. is it OK if we just loop through all steps (backwards) without prefiltering out large steps?
If not it may still be clearer to move the filtering into the loop, i.e.
for step in steps[::-1]:
if (the_condition_above_without_vectorization): continue # skip large steps
do_the_usual_thing
Note that the steps array is small anyways so there isn't much to gain from the vectorization.
I can't think of an easy way to remove the pre-filtering of I think as it stands this is a bit of an improvement - initially I was confused by |
Let's get this in. Perhaps squash first? Feel free to self-merge with or without squashing. |
steps
argument