Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Mar 28, 2023
Merged

Fix incorrect stride calculations in LogLocator.tick_values() #25405

merged 7 commits into from
Mar 28, 2023

Conversation

II-Day-II
Copy link
Contributor

PR Summary

Resolves #24092

A simplification of the stride calculation in this 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'] is False to 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

  • Has pytest style unit tests (and pytest passes)
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [N/A] New plotting related features are documented with examples.

Release Notes

  • [N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • [N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • [N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

haval0 and others added 4 commits March 1, 2023 16:44
@haval0
feat: add 2 tests based on good/bad plot from upstream issue
da5796c 
The test cases are based on the demonstration of the issue in upstream
issue matplotlib#24092

The test case for bad plot (test_tick_values_not_empty) should fail
until we implement our fix.

The test case for good plot (test_tick_values_correct) should already
suceed and continue to succeed after we implement our fix.
@haval0
fix: make "bad plot" test fail because fix isn't implemented yet
1fc5591 
"classic_mode" had to be disabled for the test to successfully replicate
the normal behavior of matplotlib. Now the test produces an empty list,
which is what should be fixed.
@haval0
Merge pull request #2 from haval0/test-good-bad-plot
01eff90 
feat: add 2 tests based on good/bad plot from upstream issue
@II-Day-II
fix: revert simplification of stride calculation in LogLocator (#3)
6d03ed8 
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 hopefully resolves matplotlib#24092.
github-actions[bot]
github-actions bot reviewed Mar 7, 2023
View reviewed changes
Copy link

@github-actions github-actions bot left a 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 us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@oscargus
Copy link
Member

oscargus commented Mar 7, 2023

Seems to be some minor numerical errors. Maybe use assert_array_almost_equal instead?

@haval0
fix: make tests slightly error tolerant (#4)
503356e 
uses the assert_almost_equal function instead of assert_array_equal
@rcomer
Copy link
Member

rcomer commented Mar 7, 2023

assert_allclose is now recommended in the numpy docs - see note here:
https://numpy.org/doc/stable/reference/generated/numpy.testing.assert_array_almost_equal.html#numpy-testing-assert-array-almost-equal

@II-Day-II
Copy link
Contributor Author

Other tests in the file use assert_almost_equal, so we try that instead.

@QuLogic QuLogic requested a review from anntzer March 7, 2023 23:15
@anntzer
Copy link
Contributor

anntzer commented Mar 8, 2023

Seems reasonable, but going through the algebra again I think the correct stride value is simply numdec // numticks + 1 (without the +1 on the numerator)? This seems to fix the issue and to pass the tests too.

@II-Day-II
Copy link
Contributor Author

Ok! That's more efficient for sure, we'll change it and make another commit.

anntzer
anntzer reviewed Mar 8, 2023
View reviewed changes
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.

post-ci.

@QuLogic
Copy link
Member

QuLogic commented Mar 21, 2023

post-ci.

@anntzer was that supposed to be an approval?

@anntzer
Copy link
Contributor

anntzer commented Mar 21, 2023

Woops, indeed.

@jklymak jklymak merged commit 9e52bc8 into matplotlib:main Mar 28, 2023
@jklymak
Copy link
Member

jklymak commented Mar 28, 2023

Congratulations on your first successful PR!

@QuLogic QuLogic added this to the v3.8.0 milestone Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@anntzer anntzer anntzer approved these changes

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
topic: ticks axis labels
Projects
First Time Contributors
Status: Merged
Milestone
v3.8.0
Development

Successfully merging this pull request may close these issues.

[Bug]: LogLocator with subs argument fragile.
8 participants
@II-Day-II @oscargus @rcomer @anntzer @QuLogic @jklymak @melissawm @haval0