-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Correctly re-instate Matplotlib converters #27481
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
Conversation
Thanks for this!
Testing these is quite hard (we accidentally broke the warning for a release and didn't notice).
|
A release note in whatsnew/0.25.1.rst would be nice to have. I think there's a "plotting" subsection. |
Codecov Report
@@ Coverage Diff @@
## master #27481 +/- ##
===========================================
+ Coverage 42.44% 91.62% +49.17%
===========================================
Files 184 184
Lines 50389 50388 -1
===========================================
+ Hits 21389 46167 +24778
+ Misses 29000 4221 -24779
Continue to review full report at Codecov.
|
Thanks for the test tip, the new test fails before the changes in this PR. |
ad83bef
to
81ec586
Compare
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.
also pls merge master
I'm away for the next week or so without a computer, so if anyone wants this in quicker than that feel free to force push to my branch. |
Is there anything else (apart from a rebase) I need to do on this? |
81ec586
to
559f258
Compare
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.
Looks good. Ping on green.
Did you merge master before pushing? We've seen similar CI failures, but I thought we fixed them all on master. |
559f258
to
9c00578
Compare
Yeah, I should have rebased on to master, I've done another rebase to see if that fixes the tests. |
Merged master again to see if we can get the CI passing. |
e5d4af2
to
08215e8
Compare
Thanks @dstansby. Sorry about the CI issues. |
Fixes #27479. Converters should only be cached to be re-instated if they are original, and not pandas converters.
I'm struggling to write a test for this; I think it requires a clean state with no pandas imports, so one can check what was in the units registry before pandas was imported. I think this means a new test file with no pandas imports at the top, but maybe someone has a better idea?