Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix BoundaryNorm cursor data output#22835
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode of Conduct.
Uh oh!
There was an error while loading.Please reload this page.
I don't have tests for inhomogeneously spaced boundaries. Shall I include some? |
On one hand you could argue that more tests are always better (because tests are better than on tests), but that fails in the same way that "if 1 apple is good, then N+1 apples is always better!" fails. More tests take more time to run, more places that things unrelated to the test can go wrong etc. So, the judgement is if the non-uniform test will exercise something important that is not exercised in the uniform case? |
Not that I can think of. |
| # map range -1..1 to 0..256 in 0.01 steps | ||
| fig, ax = plt.subplots() | ||
| fig.suptitle("-1..1 to 0..256 in 0.001") | ||
| cmap = cm.get_cmap('RdBu_r', 2000) |
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.
Looks like you are not using this variable? (And therefore not the cm import.) Nor the similar in the previous section.
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.
You are right. Fixed it! (Though, I think resampling the colormap to match the number of regions only makes a difference when viewing the plots.)
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.
I also think so, so the alternate fix of just removing thecmap may have been preferred. But now these tests will also serve as examples, so I am not strongly in favor in removing it.
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.
Me too, that's why I left them in. The tests are more complete this way and produce expected results if anyone decided to look at the plots, or took my tests as examples.
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.
I think the fix looks good!
One could possibly consider simplifying/shortening the test code a bit:
- Direct creation of test data (instead of assigning separate elements)
- Removing cmap (as discussed earlier)
- Skipping figure titles
- No need to
closeas next plot is created in a newfig
However, apart from the first point, the rest are useful when running the code in a terminal. Not sure what the consensus is on this (although I guess no one would object if they weren't there).
Is there some reason to not just use this approximation to make BoundaryNorm (non-uniquely) invertible? |
This actually is a regression and should be fixed ASAP... |
My PR doesn't include this functionality. It depends on the data being available and works around the inversion by using argmin. |
| @@ -372,3 +374,117 @@ class MyArtist4(MyArtist3): | |||
| pass | |||
| assert MyArtist4.set is MyArtist3.set | |||
| def test_format_cursor_data_BoundaryNorm(): | |||
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.
Why are you testing this intest_artist? are there other format tests in test_artist?
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.
Because my changes are inartist.py. What's a better place?
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.
... fair!
lib/matplotlib/tests/test_artist.py Outdated
| norm = mcolors.BoundaryNorm(np.linspace(-1, 1, 20), 256) | ||
| img = ax.imshow(X, cmap='RdBu_r', norm=norm) | ||
| for v in X.flat: | ||
| label = "[{:-#.{}g}]".format(v, cbook._g_sig_digits(v, 0.1)) |
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.
This test just tests that the code is called, not that the resulting labels are correct. Please consider writing out explicitly what the labels should be and comparing those.
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.
Ok, but I do this fixture for all the tests. Yes, I do in principle only test, if thecbook._g_sig_digits is called. But the important check is, if it is called with the correct arguments. My PR only prepares the values for the arguments.
Instead of comparing the output ofArtist.format_cursor_data I could also just test if thedelta argument is correct. But as it is now, I also test if the output for the cursor data is actually generated correctly. I do compare with a predefined label, though not written out explicitly but generated via a customcbook call.
So I don't know what to do? Do you want me to just replace l. 398 with the verbatim output of the format call?
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.
So I don't know what to do? Do you want me to just replace l. 398 with the verbatim output of the format call?
Yes, I think so. In general it is bad for the tests to include private methods, though that is a rule meant to be broken...
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.
Agreed, it is more understandable that way. But I left the original lines with thecbook as comments.
the fix looks good. The tests are circular, and I'm not sure ware in correct place. Moved to draft, feel free to mark "ready" when addressed Thanks! |
I'll merge once CI is done - feel free to squash, or we will squash for you at merge.... |
Thanks@yumasheta ! |
…835-on-v3.5.xBackport PR#22835 on branch v3.5.x (Fix BoundaryNorm cursor data output)
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
This shouldfix#21915 with an
isinstance(norm, BoundaryNorm)check and special logic to figure out thedeltafor calculating the value for the significant digits to show as cursor data output.I opted to use the boundaries of the
BoundaryNormdirectly, instead of calculating the midpoints of neighboring regions (midpoint is done in the non-BoundaryNormcases).I also added tests in
lib/matplotlib/tests/test_artist.py::test_format_cursor_data_BoundaryNorm. For the tests I compare the cursor data output to identically formatted strings with fixeddeltataken as the stepping ofnp.linspacethat is used to create the boundaries forBoundarNorm. The fixeddeltavalues are not identical to what this PR computed, but should give identical output for the strings.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).