FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ticker.EngFormatter: allow offset by doronbehar · Pull Request #28495 · matplotlib/matplotlib · GitHub

ticker.EngFormatter: allow offset - #28495

Merged
ksunden merged 10 commits into
matplotlib:mainfrom
doronbehar:tickerEngOffset
Oct 30, 2024
Merged

ksunden merged 10 commits into
matplotlib:mainfrom
doronbehar:tickerEngOffset

Conversation

Copy link
Copy Markdown
Contributor

PR summary

Solve #28463 by making ticker.EngFormatter be a subclass of ticker.ScalarFormatter (and not upon ticker.Formatter).

PR checklist

Comment thread lib/matplotlib/ticker.py Outdated
tacaswell added this to the v3.10.0 milestone Jul 3, 2024
Comment thread lib/matplotlib/ticker.py

Copy link
Copy Markdown
Contributor Author

Thanks for all the suggestions! I hope the documentation related comments will come out OK.

doronbehar force-pushed the tickerEngOffset branch 2 times, most recently from 696f0b1 to a2c766b Compare July 3, 2024 20:11

Copy link
Copy Markdown
Contributor Author

The last commit changes this:

Into this:

Copy link
Copy Markdown
Member

Does that change the behavior when the user does not opt-in to using offsets?

Copy link
Copy Markdown
Contributor Author

OK so after thinking about it once more, I realized that what I implemented in the demo at #28463 may be too complex - there is no need to show an order of magnitude of the data at the offset text, if we already have the ENG_PREFIXES that can shorten the strings of the ticks. Hence I managed to further simplify the logic in EngFormatter such that self.orderOfMagnitude is always 0, and the offset text only shows offset prefixed with +, and not showing the oom of the data after offset subtraction.

The only debatable drawback of this behavior, is that a 0 tick will always appear without a unit prefix. It sort of makes sense because 0 is 0 no matter what is the data's general oom. I got convinced after playing a bit, that making sure that all ticks have the same unit prefix is making the class so much more complex that it doesn't worth it. The current behavior is consistent and well tested.

The last force push simplifies as described above, and improves the test I added for this feature.

Copy link
Copy Markdown
Contributor Author

OK so after testing many more edge cases, I revised the behavior of this I changed my opinion once more. The tests now are much more extensive and I finally managed to reach the behavior I was aiming for.

The git history got too messy and I had a few reverts locally so I forced push.

Here are a few screenshots:

+/-0.5e6

+/- 3.1e3

+/- 2.1e5

Also it is automatically updated when zooming in

Offset enabled in principal, but data doesn't qualify to get an offset:

(Also when started like that, offset is computed automatically and presented if needed)

github-actions Bot added the Documentation: examples files in galleries/examples label Jul 4, 2024

Copy link
Copy Markdown
Contributor Author

I almost lost in the rebase the fixes to the comments mentioned above. They are fixed now. Hopefully docs will build fine now 🙏.

Copy link
Copy Markdown
Contributor Author

I'm having some trouble building the documentation locally, so I have to rely upon CI. Last force push fixed a few small issues with it.

QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Mostly minor stuff, but a couple of small implementation questions.

Comment thread lib/matplotlib/ticker.py Outdated
Comment thread lib/matplotlib/ticker.py Outdated
Comment thread lib/matplotlib/ticker.py
Comment thread lib/matplotlib/ticker.py Outdated
Comment thread lib/matplotlib/ticker.py Outdated

QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry, forgot to check the tests.

Comment thread lib/matplotlib/tests/test_ticker.py Outdated
Comment thread lib/matplotlib/tests/test_ticker.py Outdated
Comment thread lib/matplotlib/tests/test_ticker.py Outdated
Comment thread lib/matplotlib/tests/test_ticker.py Outdated
Comment thread lib/matplotlib/tests/test_ticker.py Outdated
Comment thread lib/matplotlib/tests/test_ticker.py Outdated
Comment thread lib/matplotlib/tests/test_ticker.py Outdated

Copy link
Copy Markdown
Contributor Author

Thank you @QuLogic for the detailed review! I don't have time now to respond to all of your suggestions, so I started with the easier parts, documentation and comments.

doronbehar force-pushed the tickerEngOffset branch 2 times, most recently from 2646527 to 1290331 Compare October 13, 2024 09:52

Copy link
Copy Markdown
Contributor Author

OK so last night I struggled a bit running the tests locally, so I hoped my changes would just work. Now I finally checked locally before pushing. I also responded to all other comments, and the topics left to discuss are the local variables helping to debug with pytest --showlocals, and meaning of oom_ variables in the test_engformatter_offset_oom, and:

`matplotlib.ticker.EngFormatter`

v.s

:class:`matplotlib.ticker.EngFormatter`

In the docs (not in the titles of documents - that's understandable).

doronbehar commented Oct 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I don't understand the circle CI errors... They don't seem related to my changes. Same with the errors on Windows and MacOS on Azure.

QuLogic commented Oct 16, 2024

Copy link
Copy Markdown
Member

I don't understand the circle CI errors... They don't seem related to my changes. Same with the errors on Windows and MacOS on Azure.

It appears you have rebased against a very old commit instead of the current main.

Copy link
Copy Markdown
Contributor Author

Rebased.

Copy link
Copy Markdown
Contributor Author

Less errors now, but I see now:

Path does not exist: /Users/runner/work/1/s/result_images

Comment thread lib/matplotlib/tests/test_ticker.py Outdated
Comment thread lib/matplotlib/tests/test_ticker.py Outdated
Comment on lines +1644 to +1646
# These prefix_ variables are used only once, so we could have inlined
# them all, but it is more comfortable in case of tests breakages to
# view their values with pytest --showlocals.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That pytest's assertion rewriting isn't showing you a breakdown of the results is a bug, and --showlocals is a reasonable workaround, but I wouldn't leave a comment about it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That pytest's assertion rewriting isn't showing you a breakdown of the results is a bug, and --showlocals is a reasonable workaround, but I wouldn't leave a comment about it.

What do you mean by pytest's assertion rewriting?

Copy link
Copy Markdown
Contributor Author

The mypy CI error doesn't seem related to the PR...

ksunden commented Oct 30, 2024

Copy link
Copy Markdown
Member

Mypy passes locally, CI failure was fixed by #29014, would need a rebase to resolve on CI, butr since it passes locally and the failure is understood, I'm satisfied.

ksunden merged commit 61833b8 into matplotlib:main Oct 30, 2024

Copy link
Copy Markdown
Contributor Author

Thank you for your corporation :).

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL