Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix incorrect stride calculations in LogLocator.tick_values()#25405
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The test cases are based on the demonstration of the issue in upstreamissuematplotlib#24092The test case for bad plot (test_tick_values_not_empty) should failuntil we implement our fix.The test case for good plot (test_tick_values_correct) should alreadysuceed and continue to succeed after we implement our fix.
"classic_mode" had to be disabled for the test to successfully replicatethe normal behavior of matplotlib. Now the test produces an empty list,which is what should be fixed.
feat: add 2 tests based on good/bad plot from upstream issue
The attempt to simplify the stride calculation in `a06f343dee3cebf035b74f65ea00b8842af448e9` seems to be the cause of the problem. Using an approach equivalent to what was there before hopefullyresolvesmatplotlib#24092.
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
Seems to be some minor numerical errors. Maybe use |
uses the assert_almost_equal function instead of assert_array_equal
|
Other tests in the file use |
Seems reasonable, but going through the algebra again I think the correct stride value is simply |
Ok! That's more efficient for sure, we'll change it and make another commit. |
mnt: simplify stride calculation in LogLocator.tick_values
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.
post-ci.
@anntzer was that supposed to be an approval? |
Woops, indeed. |
Congratulations on your first successful PR! |
PR Summary
Resolves#24092
A simplification of the stride calculation inthis commit seems to have introduced some incorrect behavior, as the simplified version isn't mathematically equivalent to the old one. This caused the method to return an empty array when it shouldn't, which meant ticks would not appear on the grid axes.
This PR reverts the stride calculation in the case where
mpl.rcParams['_internal.classic_mode']isFalseto one equivalent to the while-loop the simplification replaced, as well as adding tests for the scenario described in#24092.PR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst