| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Thanks for all the suggestions! I hope the documentation related comments will come out OK. |
Sorry, something went wrong.
Sorry, something went wrong.
|
Does that change the behavior when the user does not opt-in to using offsets? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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.1e5Also it is automatically updated when zooming inOffset 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) |
Sorry, something went wrong.
|
I almost lost in the rebase the fixes to the comments mentioned above. They are fixed now. Hopefully docs will build fine now 🙏. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Mostly minor stuff, but a couple of small implementation questions.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, forgot to check the tests.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
It appears you have rebased against a very old commit instead of the current main. |
Sorry, something went wrong.
|
Rebased. |
Sorry, something went wrong.
|
Less errors now, but I see now: Path does not exist: /Users/runner/work/1/s/result_images |
Sorry, something went wrong.
| # 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. |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
Allows us to use many order of magnitude and offset related routines from ScalarFormatter, and removes a bit usetex related duplicated code. Solves matplotlib#28463.
|
The mypy CI error doesn't seem related to the PR... |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Thank you for your corporation :). |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR summary
Solve #28463 by making ticker.EngFormatter be a subclass of ticker.ScalarFormatter (and not upon ticker.Formatter).
PR checklist