-
Notifications
You must be signed in to change notification settings - Fork 279
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
RFC: a more robust plot norm/colorbar API #3849
Conversation
87e9f0b
to
91cde83
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.
A few thoughts and questions before running off!
@@ -883,7 +932,7 @@ As an example, | |||
|
|||
ds = yt.load_sample("FIRE_M12i_ref11") | |||
p = yt.ProjectionPlot(ds, "x", ("gas", "density")) | |||
p.set_log(("gas", "density"), True, symlog_auto=True) | |||
p.set_log(("gas", "density"), linthresh="auto") |
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.
Is symlog_auto
still allowed?
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.
yes, but I'm deprecating it
yt/visualization/_commons.py
Outdated
@@ -93,3 +97,128 @@ def get_canvas(figure, filename): | |||
f"without an extension." | |||
) | |||
return get_canvas_class(suffix)(figure) | |||
|
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.
Are these functions inspired by or taken from somewhere else?
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.
It's all moved code:
- the
get_x_minorticks
routines are taken (moved) fromyt.visualization.plot_container
get_symlog_majorticks
is extracted fromImagePlotMPL._init_image
get_default_from_config
is adapted from a closure inPlotContainer.setup_defaults
The may reason for moving them around was to help me remembering were everything I needed was defined, but I may restore some of them to their original location when I try to clean this branch a bit.
6a7b850
to
4f4be55
Compare
Some desired changes in image tests collide with #3818, which is set for backporting, so I'll wait for that PR to resolve before I open this one for review. |
159260d
to
1f03523
Compare
I think I got this in a state were all remaining failures are expected changes. I simplified the branch history and reduced the amount of commits by a factor 10. |
Just noticed that some docs example like https://yt-project.org/doc/cookbook/simple_plots.html#showing-and-hiding-axis-labels-and-colorbars |
d8f07ab
to
3ffc29b
Compare
Docs build got stuck |
f245a0b
to
fd4a6cc
Compare
5ca8dd3
to
0a20134
Compare
4ad92e6
to
e7b17cd
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.
Again, many apologies for this taking so long to review. In general, I like what is done here. The old methods for specifying colorbar normalizations were getting pretty clunky after I added the auto_symlog
kwarg a while back, and this is definitely an improvement over that. I tried to review as much as I could, but there is a lot of code here. In general, I just ran some test scripts locally to ensure that they worked and produced what was expected and read the code looking for any obvious inconsistencies.
My main comments are on how this is documented to ensure the users know how to use it, since it's changing the defaults a bit. I was hoping we could change references to "norms" in the docs to "normalizations" or "scalings", since I think norms is a bit confusing. But I realize that the classes and methods you created reference "Norms", which I think is fine. What do you think? After that minor stuff gets addressed, I'm happy to approve and merge this.
doc/source/visualizing/plots.rst
Outdated
Colorbar norms | ||
:::::::::::::: | ||
|
||
Slice plots and similar plot classes default to log norms when all values are |
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.
Have profile plots also moved to the new format--from below it looks like it? Maybe mentioning that here too?
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.
Profile plots don't have a colorbar, unless you mean specifically 2D profile plots, aka phase plots ? (and yup, should mention it here)
Co-authored-by: Cameron Hummels <chummels@gmail.com>
af88250
to
f3d82d7
Compare
@chummels thanks ! I think I replied to all your comments so far, but I'll let you resolve the threads when you're satisfied. Just a note that merging this will not be a one step process. We need to merge the answer-store PR first, then update here. |
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.
Thanks for addressing my concerns. Psyched to see this go in! Only a couple of little requests, but other than that, this is great.
Co-authored-by: Cameron Hummels <chummels@gmail.com>
Alright, looks like we're go ! Thank you so much for going through this gigantic PR |
Ok this is now ready at last ! @chummels, if you'll do the honours ? 😄 |
The docs build failed, but I'm hoping it's a random server-side event:
Let's try again: @yt-fido test this please |
PR Summary
This is a deep refactor of how colorbar and plot norm data are stored and utilized internally.
It allows for a much more robust API where chaining operations like
set_zlim
andset_unit
doesn't ruin the whole viz (fix #2538).It also makes room to implement stuff like custom norm (fix #3840), and helps resolve a documented quirky behaviour (fix #3852).
It unifies the various strategies scattered across plot classes to determine what norm can be used in a given situation (symlog, log, linear ?), which is a area where we've seen many bugs in the recent pastn, basically on every matplotlib feature release.
I've been iterating and testing on this for a couple weeks locally, I'm opening the PR while it's still hot to see what bug existing tests might reveal.
Documentation is already included, but for now I haven't committed my test files because they are not adapated to yt's image comparison testing framework yet.
Note that this includes the changes from #2504, which changes some phase plots with respect to their gold standard, so CI should fail at the very least there, but no other failures are desired.
edit: I'm bumping matplotlib's minimal required version to 3.1 because 2.2.3 has some incompatibilities with the refactor and 3.1 is the oldest testable version for macOS.
FTR Matplotlib 3.1 was released in May 2019 and was only distributed for Python 3.6 and 3.7, so it's still a pretty conservative requirement now.
Note to reviewers
I opened a "companion" PR at #3900, which contains only the new tests, not the patch, to help demonstrate what's working here and not on the main branch.
I also tried to keep the commit history somewhat tidy and readable, so reading commits by chronological order is one possible angle to review this in steps.
After a lot of iterations, I bumped/added new answers both on Jenkins and the answer store. The supporting PR is yt-project/answer-store#31
A Summary of user-facing changes
A lot of symlog-related code is now centralised, making it reusable to all
PlotContainer
subclasses (not justImagePlotContainer
).api changes
ImagePlotContainer.set_zlim
to :zmin
orzmax
only (previously they had to be both specified somehow)unyt_quantity
objects or parsable tuples e.g.(10, "g/cm**2")
)PlotContainer.set_log
so that the second argument (log
) isn't necessary when passinglinthresh
. Previously users had to provide a value to thelog
argument but it was not actually used. Now a warning is emitted if alog
value is received but discarded (this is however not deprecated).PlotContainer.set_norm
method to allow using matplotlib norms other than lin, log and symlogdeprecations
zmin=None
(respzmax=None
) toset_zlim
explicitly is deprecated (users can leave the parameter unset or passzmin="min"
(respzmax="max"
) explicitly instead)PlotContainer.set_log
'ssymlog_auto
parameter is deprecated (the same behaviour can be obtained by passinglinthresh="auto"
instead)dependencies
typing_extensions
(which are backports from the standard lib'styping
module), exclusively for Python 3.7 (we can rely fully on the std lib in 3.8 and beyond)PR Checklist
TODO(clm)
)The following PRs should be handled before this one: