Skip to content

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

Merged
merged 3 commits into from
Feb 10, 2023
Merged

Clean + comment MaxNLocator #25166

merged 3 commits into from
Feb 10, 2023

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Feb 6, 2023

  • Remove redundant "always positive" comment (abs is always always positive!)
  • Improve docstring for steps argument
  • Clean up logic for filtering the step sizes in the classic autolimit mode. I thought using a loop for this before was quite opaque, so hope what I've done here is clearer?
  • Add a comment above the loop through steps, explaining what it's doing.

Copy link
Member

@oscargus oscargus left a 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...)

@dstansby dstansby requested a review from anntzer February 8, 2023 09:52
Copy link
Contributor

@anntzer anntzer left a 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.

@dstansby
Copy link
Member Author

dstansby commented Feb 10, 2023

I can't think of an easy way to remove the pre-filtering of large_steps. The core issue is that we have an array that looks like [..., False, False, True, True...], and want to start at the index of the first True entry in the array and work backwards through the False indicies. In order to do that I think there has to be some sort of pre-computation before the main loop to deterimine where this is.

I think as it stands this is a bit of an improvement - initially I was confused by istep being modified by the loop under if mpl.rcParams['axes.autolimit_mode'] == 'round_numbers':, and instead modifying the mask and then getting istep is a bit clearer. Very happy for other suggestions to improve more though, I definitely agree it's still not terribly clear.

@anntzer
Copy link
Contributor

anntzer commented Feb 10, 2023

Let's get this in. Perhaps squash first? Feel free to self-merge with or without squashing.

@dstansby dstansby merged commit 250a8bb into matplotlib:main Feb 10, 2023
@dstansby dstansby deleted the maxnlocator branch February 10, 2023 16:17
@QuLogic QuLogic added this to the v3.8.0 milestone Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants