Fix boundary norm negative #21676
Fix boundary norm negative #21676
Conversation
lib/matplotlib/colorbar.py
Outdated
| @@ -1150,7 +1150,7 @@ def _mesh(self): | |||
|
|
|||
| def _forward_boundaries(self, x): | |||
| b = self._boundaries | |||
| y = np.interp(x, b, np.linspace(0, b[-1], len(b))) | |||
| y = np.interp(x, b, np.linspace(0, np.abs(b[-1]), len(b))) | |||
There was a problem hiding this comment.
Can you clarify what is being computed here? Without context, it seems wrong that only the last point of b is taken into account, in particular the example in #21671 fails again (with this PR) if b[-1] == 0 (e.g., clevs = list(range(-94, 1))), or rather the resulting plot is wrong.
There was a problem hiding this comment.
Good catch. Fixed. OTOH do we know if clevs=np.arange(1, -9, -1) should be allowed?
There was a problem hiding this comment.
I would like decreasing levels to be allowed, but to be clear that's a separate issue that can be dealt with later; the problem here is with increasing levels where the last level is zero.
There was a problem hiding this comment.
Yes, I can confirm that clev=np.arange(1, -9, -1) used to return nonsense on v3.4.3. It errors on this PR as of my latest push, which seems better than nonsense, but it can probably be made to work.
There was a problem hiding this comment.
Would it work to just set the second arg to range(len(b)) instead?
There was a problem hiding this comment.
though now that I look at it y[x > b[-1]+eps] = 2 can't possibly be right...
There was a problem hiding this comment.
Yes, I meant the third. I'm also a bit confused by the setting to -1/+2 for out-of-bounds values...
There was a problem hiding this comment.
No thats clearly wrong. Fixed in the latest. However, I played with this, and there are no tests of this happening, so I'm not sure where this hedging came from. I think it may have been extra ticks in the extend region, but that can't really happen (because its just bounds mapping to bounds) so maybe we don't need all this...
There was a problem hiding this comment.
I see that in the new version you're mapping to np.linspace(0, db, len(b)), which, while not "wrong", still seems weird to me (I still think the mapped-to space should be either range(len(b)) or np.linspace(0, 1, len(b)) -- db doesn't have any particular meaning in that space). And then I'd map the out of bounds points to -1 and len(b) (first case) or -1/(len(b)-1) and 1+1/(len(b)-1) (second case) (which I guess argues that the first representation is simpler).
There was a problem hiding this comment.
OK, I think that works...
6423cc5 to
6d329fa
Compare
|
Just to describe what this does so I'm not just hacking...
I don't think this needs an image test because we do visually test the boundary norm, just we weren't doing it with negative levels, so the proposed test should be adequate. |
c0e7110 to
7ce4a7e
Compare
|
@anntzer This version (boundaries mapped from 0-1) fails on pixel snapping of ticks at Image instability because of pixel snapping is pretty annoying... |
|
Which one is "better" for you? (sorry, that was a bit unclear for me in your message) I looked at the images; it seems that the difference basically arises because the boundary between two color blocks occurs between two pixel rows, and it's just a matter of deciding whether the tick is drawn on the last row of the first block or the first row of the second block, which is a bit arbitrary? I think I'd just stick a tol=... on that test (as we know where the discrepancy comes from) and be done with it? |
|
Yep - if we just tested PDFs that might be better - bitmapping is done visually on the PDF, rather than with our numerically unstable snapping behaviour in the AGG backends. However, testing PDFs is slower... |
|
Let's just live with what we have for now? :) |
|
(Yes, but in general I've been favouring png-only tests a lot of time, whereas now I think maybe we should favour pdf-only... Though pdf sometimes has its own problems. ) |
34329d2 to
089430c
Compare
|
squashed.... |
…676-on-v3.5.x Backport PR #21676 on branch v3.5.x (Fix boundary norm negative)
'visually on the PDF' is just rasterizing at a different step, and is dependent on the rasterizer in use. I'm not sure this is any safer given PDFs sometimes break when updating Ghostscript, even if visually you see no difference. |
PR Summary
Closes #21671
Fixes an issue where the BoundaryNorm colorbar was broken if the boundaries were negative. Added new test to make sure this doesn't break again.
I'll be honest, I find this part of the colorbar code a bit inscrutable, and it doesn't have a lot of unit tests. So more bug reports to help get all the combinations down would be helpful.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).