Skip to content
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

ENH: add support for symlog colorbars with arbitrary bases (requires MPL>=3.5) #4449

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented May 21, 2023

PR Summary

replay #4447 + closes #4448

test_sym

While the set of minor ticks on a log scale isn't well defined in the general case (say, natural log), I find that our existing approach for log10 (get_symlog_(minor|major)ticks()) can be generalized to work with any integer base. This is essentially what I'm doing here, using pure MPL API. For non-integer bases, I delegate to matplotlib (which is equivalent of giving up minor ticks).

As noted in #4448, I found it was very hard (likely impossible) to fix the issue with matplotlib older than 3.5, so for these versions, I'm keeping bug-for-bug compatibility.

TODO:

I also opened matplotlib/matplotlib#25994 but the further I dig in, the less likely it looks that this will have a swift, short term solution upstream, so I'm not making its resolution a requirement for this PR.

@neutrinoceros neutrinoceros added the enhancement Making something better label May 21, 2023
@neutrinoceros neutrinoceros force-pushed the fix_symlog2 branch 2 times, most recently from db179ec to 8b41771 Compare May 22, 2023 09:03
@neutrinoceros
Copy link
Member Author

Still having 4 errors in pytest-mpl tests, 3 of which are identical and all of which result from leaning more on matplotlib to determine major ticks in symlog, and only affect edge cases. I believe these differences are acceptable (desirable, even).

Next step will be to open a discussion with matplotlib devs to see which part of this work (if any) they'd be interested in upstreaming.

@matthewturk
Copy link
Member

This looks good to me, and great work proposing it for upstreaming.

@neutrinoceros
Copy link
Member Author

pre-commit.ci autofix

@neutrinoceros
Copy link
Member Author

Turns out merely keeping bug-for-bug compatibility for MPL<3.5 adds a lot of complexity to ImagePlotMPL._set_axes, which IMO hurts maintainability and probably makes reviewing this PR much harder. It would make my life much easier to just require 3.5 everywhere.

@neutrinoceros
Copy link
Member Author

Alright this is now stable enough for review. All remaining image failures are expected and intended, the companion PR for the baseline submodule may be found at yt-project/yt_pytest_mpl_baseline#3

@neutrinoceros neutrinoceros marked this pull request as ready for review May 29, 2023 10:00
@neutrinoceros neutrinoceros added this to the 4.3.0 milestone May 29, 2023
@matthewturk matthewturk merged commit a333673 into yt-project:main Jun 9, 2023
1 check passed
@neutrinoceros neutrinoceros deleted the fix_symlog2 branch June 9, 2023 19:02
@neutrinoceros
Copy link
Member Author

@matthewturk the companion PR should have been merged first ! I'll open a follow up PR to fix tests on main

neutrinoceros added a commit to neutrinoceros/yt that referenced this pull request Jun 9, 2023
matthewturk added a commit that referenced this pull request Jun 9, 2023
TST: bump pytest-mpl tests (follow up to GH #4449)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better viz: 2D
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: cannot set colorbar norm to SymLogNorm with base !=10
2 participants