GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix incorrect stride calculations in LogLocator.tick_values() by II-Day-II · Pull Request #25405 · matplotlib/matplotlib

Conversation

@II-Day-II

@II-Day-II II-Day-II commented Mar 7, 2023

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@melissawm melissawm added the topic: ticks axis labels label Mar 7, 2023
@oscargus

oscargus commented Mar 7, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@haval0
fix: make tests slightly error tolerant (#4)
503356e 
uses the assert_almost_equal function instead of assert_array_equal
@rcomer

rcomer commented Mar 7, 2023

Copy link
Copy Markdown
Member

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
oscargus reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@II-Day-II

II-Day-II commented Mar 7, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@QuLogic
QuLogic requested a review from anntzer March 7, 2023 23:15
@anntzer

anntzer commented Mar 8, 2023

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@II-Day-II

II-Day-II commented Mar 8, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
II-Day-II and others added 2 commits March 8, 2023 16:47
@II-Day-II
mnt: simplify stride calculation in LogLocator.tick_values
874e6ea
@haval0
Merge pull request #5 from haval0/do-it-with-algebra
e1813b5 
mnt: simplify stride calculation in LogLocator.tick_values
anntzer
anntzer reviewed Mar 8, 2023
View reviewed changes

@anntzer anntzer left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@QuLogic

QuLogic commented Mar 21, 2023

Copy link
Copy Markdown
Member

post-ci.

@anntzer was that supposed to be an approval?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anntzer
anntzer approved these changes Mar 21, 2023
View reviewed changes
@anntzer

anntzer commented Mar 21, 2023

Copy link
Copy Markdown
Contributor

Woops, indeed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jklymak
jklymak approved these changes Mar 28, 2023
View reviewed changes
@jklymak
jklymak merged commit 9e52bc8 into matplotlib:main Mar 28, 2023
@jklymak

jklymak commented Mar 28, 2023

Copy link
Copy Markdown
Member

Congratulations on your first successful PR!
II-Day-II and haval0 reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@QuLogic QuLogic added this to the v3.8.0 milestone Mar 28, 2023
davidgegiya added a commit to davidgegiya/agent-orchestrator that referenced this pull request Nov 9, 2026
@davidgegiya
Add matplotlib #25405: LogLocator with subs argument fragile
66681b4 
Pin 84e5f0251cea920d02b631646bd8832bc0e489e5 and its regression selectors for reproducible runs.

Source: matplotlib/matplotlib#25405
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@II-Day-II @oscargus @rcomer @anntzer @QuLogic @jklymak @melissawm @haval0
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.