Skip to content

Revert #23417 (Consistently set label on axis with units) #25278

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
Feb 22, 2023
Merged

Revert #23417 (Consistently set label on axis with units) #25278

merged 7 commits into from
Feb 22, 2023
+22 −30

Conversation

ksunden
Copy link
Member

@ksunden ksunden commented Feb 21, 2023 •
edited
Loading

  • Revert "Fix logic line break"
  • Revert "Improve unit axis label test"
  • Revert "Make have_units_and_converter private"
  • Revert "Update units_rectangle baseline image"
  • Revert "Fix unit info setting axis label"
  • Revert "Add test for axis label when units set"

PR Summary

PR #23417 caused problems for downstream libraries which provide unit converters.

The most prominent example is pandas, as discussed in depth in #25219 (edit: fixed linked discussion issue).
While there are other solutions that solve the narrow case, it was determined there that the pragmatic answer was to revert and sort out the details as a follow up.

PR Checklist

Documentation and Tests

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

Release Notes

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

dstansby
dstansby previously requested changes Feb 21, 2023
View reviewed changes
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to revert (sorry for breaking!), but I think at the same time we should put in a test that breaks before this PR was reverted, and passes afterwards, so we don't accidentally break things in the future.

@dstansby dstansby dismissed their stale review February 21, 2023 22:33

Don't have time to review the test now, but I'll remove my change request, thanks for adding a test!

@ksunden
Copy link
Member Author

ksunden commented Feb 21, 2023

Test added, it is a little bit apocryphal, as I don't really expect such things to be done with built in converters, but rather than introducing test-only subclasses, it still gets at the mechanism that I think is at the heart of the issue in #25219, that if a valid converter is applied to an Axis, it should not get overridden when further changes (such as xlim) are applied to the axes (which otherwise work with that converter), which is what was happening to Pandas' converters (though with the more drastic change to string categoricals as their date converters accept strings, which ours do not).

@ksunden ksunden added this to the v3.7.1 milestone Feb 22, 2023
@tacaswell tacaswell merged commit eeef80f into matplotlib:main Feb 22, 2023
@meeseeksmachine meeseeksmachine mentioned this pull request Feb 22, 2023
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Feb 22, 2023
@tacaswell @meeseeksmachine
Backport PR matplotlib#25278: Revert matplotlib#23417 (Consistently s…
daccc71 
…et label on axis with units)
ksunden added a commit that referenced this pull request Feb 23, 2023
@ksunden
Merge pull request #25294 from meeseeksmachine/auto-backport-of-pr-25…
c32a671 
…278-on-v3.7.x

Backport PR #25278 on branch v3.7.x (Revert #23417 (Consistently set label on axis with units))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

@jklymak jklymak jklymak approved these changes

@dstansby dstansby dstansby left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.7.1
Development

Successfully merging this pull request may close these issues.
4 participants
@ksunden @tacaswell @jklymak @dstansby