Codecov Report

Merging #16392 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #16392      +/-   ##
==========================================
- Coverage   80.85%   80.85%   -0.01%     
==========================================
  Files         307      307              
  Lines       75745    75754       +9     
  Branches     9690     9693       +3     
==========================================
+ Hits        61245    61250       +5     
- Misses      11961    11964       +3     
- Partials     2539     2540       +1     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a2971b...e175ffc. Read the comment docs.

There are multiple PRs out fixing various portions of the SymLog routines right now. I don't think that this one makes sense because you're going to add this new image test that will just fail and need to be updated immediately when you change the base scaling as proposed in the other ones.

I think it would be helpful to take a step back and rework the entire machinery at once rather than piecemealing it together.

This is orthogonal to whether the norm is correct or not, and this basically will be the system in place once the norm is sorted out. So I agree that the image test will need to change again, but I think this small change has more chance of getting in and affects other norms rather urgently. So I'd argue thats worth the extra image in the repo.

We could also just take it on faith (aka local manual testing) that the new approach is better (after all we've been doing "fine" (or not so fine...) without a proper test for years now), push this first fix without the baseline image test, and put a proper test once everything is settled.

OK, I opened #16398 to remind us to re-add @dstansby's test when the norms are settle down, and took the image test and image out of this PR.

I think this PR should still go in to fix the problem with the bad colorbars.

I don't think we have double-underscore prefixed things anywhere in mpl, why would this one need to be more private?

paranoia

I am ambivalent towards whether its __scale or _scale. Right now I have tow underscores, but if you'd like it changed, I can easily do that.

I actually prefer single underscore quite strongly here; I don't want to start creating "two levels" of private attributes and give the impression that some (single-underscore) private attributes are "more ok" to use as semi-public API than others (double-underscore), nor do I want to have mass-PRs replacing single underscores by doubles...

I didn't really understand the argument for two underscores, but just did as @tacaswell suggested. Suggest he engages with you on this ;-)

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

$ git checkout v3.2.x
$ git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 ee2691c3f8578b48e1b796df31672a91b88f6d97

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #16392: FIX colorbars for Norms that do not have a scale.'

4. Push to a named branch :

git push YOURFORK v3.2.x:auto-backport-of-pr-16392-on-v3.2.x

5. Create a PR against branch v3.2.x, I would have named this PR:

"Backport PR #16392 on branch v3.2.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

Note that __ is not just "extra" private; in a class, it means it's not available to subclasses as well (because it's renamed to _ClassName_attribute).