RFC: a more robust plot norm/colorbar API

[neutrinoceros]

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 and set_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 https://github.com/yt-project/answer-store/pull/31

A Summary of user-facing changes

A lot of symlog-related code is now centralised, making it reusable to all PlotContainer subclasses (not just ImagePlotContainer).

api changes

• updated ImagePlotContainer.set_zlim to :
  • allow passing zmin or zmax only (previously they had to be both specified somehow)
  • allow passing unit-aware values (either as unyt_quantity objects or parsable tuples e.g. (10, "g/cm**2"))
• updated PlotContainer.set_log so that the second argument (log) isn't necessary when passing linthresh. Previously users had to provide a value to the log argument but it was not actually used. Now a warning is emitted if a log value is received but discarded (this is however not deprecated).
• added a PlotContainer.set_norm method to allow using matplotlib norms other than lin, log and symlog

deprecations

• passing zmin=None (resp zmax=None) to set_zlim explicitly is deprecated (users can leave the parameter unset or pass zmin="min" (resp zmax="max") explicitly instead)
• PlotContainer.set_log's symlog_auto parameter is deprecated (the same behaviour can be obtained by passing linthresh="auto" instead)

dependencies

• bumped minimal supported version of MPL from 2.2.3 to 3.1 (this represents less than a 1yr jump)
• added a dependency to typing_extensions (which are backports from the standard lib's typing module), exclusively for Python 3.7 (we can rely fully on the std lib in 3.8 and beyond)

PR Checklist

• [x] New features are documented, with docstrings and narrative docs
• [x] cleanup remaining todos in the code (TODO(clm))
• [x] Adds a test for any bug fixed. Adds tests for new features.
• [x] cleanup history to facilitate reviews
• [x] check that #3901 is (or stays) fixed
• [x] add a test for it

The following PRs should be handled before this one:

• [x] #2504
• [x] #3818
• [x] #3888

[neutrinoceros]

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.

[neutrinoceros]

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. I also listed 2 PRs that should be handled before this one in the OP.

[neutrinoceros]

Just noticed that some docs example like https://yt-project.org/doc/cookbook/simple_plots.html#showing-and-hiding-axis-labels-and-colorbars are still broken on this branch. I'll continue to work on it.

[neutrinoceros]

Docs build got stuck @yt-fido test this please

[neutrinoceros]

This now fixes #3895 too

edit: taking this back, I don't think the approach I took was viable in the end

[neutrinoceros]

@yt-fido test this please

[neutrinoceros]

@Xarthisius I may need a hand here. I'm trying to create new image answer tests but they don't seem to be run at all, despite the fact that I included them in test/tests.yaml. Am I doing something wrong or does it need some server side intervention ? FTR I'm planning to add a bunch more similar tests, but none of them should require any new data.

[Xarthisius]

Not sure. Answer tests without requires_ds are a little bit of mess. There are at least 3 different approaches... Try:

from nose.plugins.attrib import attr

@attr(ANSWER_TEST_TAG)
def test...

[Xarthisius]

Looks like it did the trick: https://tests.yt-project.org/job/yt_py38_git/4959/testReport/yt.visualization.tests/test_norm_api/

[neutrinoceros]

awesome, thank you !

[neutrinoceros]

So I think my tests are now running correctly on Jenkins, however I'm getting some YTNoOldAnswer errors on the GH side of answer tests. I don't think I've seen this situation yet, do you know what should be done here @Xarthisius ?

edit: actually given the evidence I've grasped with https://github.com/yt-project/yt/pull/3900, I'm not even sure anymore that what's happening on Jenkins is correct either. Specifically I'm seeing some errors being reported multiple times and others not at all.

[Xarthisius]

For github side of things, new answers are uploaded to use.yt. In the test phase called "report failed answers" look for: Successfully uploaded answer(s) for failed test at URL: http://use.yt/upload/48419d6c . Please commit these answers in the repository's answer-store.

[neutrinoceros]

Got it, thanks again ! However I have doubts that my tests are correctly implemented, see https://github.com/yt-project/yt/pull/3900 Since I'm really unsure about the steps I took up to now, I'd rather wait for some feedback there before I take the next one.

[neutrinoceros]

So I think I got this in a much better state by essentially isolating my tests (one module -> one test): The reports now look as I expect on Jenkins. However there seem to be a new problem with the GHA job: upload fails. Do you think this a server side problem, @Xarthisius ?

[neutrinoceros]

I've been trying to avoid running the new answer test on GH but so far none of my hack has worked. Meanwhile test reports still look broken, I don't know what to do.

[neutrinoceros]

I reported the CI problem as #3910

[neutrinoceros]

This is almost ready. Requesting a review from @chrishavlin with because of the proximity of his recent work :)

[neutrinoceros]

Thank you so much ! I confirm that I'm getting the same result as you are with the FIRE... example, but bear in mind this change was intentional and already merged as https://github.com/yt-project/yt/pull/3859, it was also shipped in yt 4.0.3. I agree that in this specific case the new behaviour is not ideal, but here's my reasoning

• defaults can't be perfect in every case (which is probably why matplotlib won't even attempt to determine a value for linthresh
• the previous behaviour, while more adapted to the specific example in https://github.com/yt-project/yt/pull/3295, was causing blocking problems in the niche case described in #3858
• it's preferable to have defaults that always work and can be adjusted rather than fine tuning for known cases at the cost of breaking others.

What do you think ?

I'll go over the bulk of your suggestions and comments this week end, thanks again !

[chrishavlin]

Ok! Ya, that makes sense to me. But I do think we should change the example in the docs. Could use a different dataset to demo the linthresh=auto functionality or adjust the example to show how to set linthresh values when auto doesn't do a great job?

[neutrinoceros]

I updated the symlog docs. Hopefully it's better now.

[neutrinoceros]

@yt-fido test this please

[neutrinoceros]

@chrishavlin I think we're down to only 2 threads awaiting resolution. I had to iterate a little more to get CI stable again, hopefully it should be good now (last try, the docs build failed but I'm not convinced that it's reproducible, this should be visible in about an hour from now)

[chrishavlin]

(for posterity -- found and resolved the open threads)

[neutrinoceros]

rebased (quite abusively) and added a cleanup commit to revalidate tests after #3907 was merged

[matthewturk]

I am generally inclined to defer to @chrishavlin on this one.

[chummels]

As I mentioned in slack, I am very concerned that this regresses the code from PR #3295, as @chrishavlin identified earlier in this conversation. Arbitrarily choosing to have the linthresh set to the maximum / 1000 seems problematic, particularly for cosmological data. The FIRE dataset is a good example of the many datasets that are like that, and I think it's going to a be a big problem if our defaults project a picture with a couple of dots on them instead of the fully rendered image. I think it's reasonable to have a better auto-threshold chosen.

[neutrinoceros]

Note that this change of default is already present on main. I'm open to better solutions but it seems to me that it should be a separate discussion. See my answer to Chris https://github.com/yt-project/yt/pull/3849#issuecomment-1126227079

[chrishavlin]

Arbitrarily choosing to have the linthresh set to the maximum / 1000 seems problematic, particularly for cosmological data.

yes! very much agreed. In playing around, just changing that to cover a larger dynamic range improved things quite a bit, e.g., max val / 1e10. But I'm inclined to fix this in a follow up since that change actually happened in an earlier bugfix (that was my bad for not realizing the full implications while reviewing that bug fix) -- I'll start an issue for discussion.

[neutrinoceros]

@chummels now that https://github.com/yt-project/yt/issues/3944 is dedicated to this discussion, can we move forward with this PR ?

[neutrinoceros]

seems like the latest merge from main broke a couple tests, I will address this sometimes this week