-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix checking of 'labels' argument to Sankey.add. #13218
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
Conversation
Test by manually running the sankey examples.
007dd8b
to
c2fb60f
Compare
I'm confused as to what issue this is solving; could you add a simple example to the test suite that raises the error? |
https://matplotlib.org/devdocs/gallery/specialty_plots/sankey_basics.html#sphx-glr-gallery-specialty-plots-sankey-basics-py and https://matplotlib.org/devdocs/gallery/specialty_plots/sankey_basics.html#sphx-glr-gallery-specialty-plots-sankey-basics-py are currently completely misrendered; this PR fixes that. |
The test suite can't tell if the docs are being rendered correctly; correct me if I'm wrong, but adding a test that exercises the |
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 this needs a test that exercises the ValueError
But the ValueError is not actually the relevant point; what matters is the fact that if An image test would be better, but if I was to add one I'd rather design a proper test that actually ensures as close to 100% coverage as possible on a single image test (which I don't have right now), rather than repeatedly trying to cover incrementally more parts of the module. |
Fair enough re. an image test. Would it be possible to create a diagram, then extract the resulting labels to check they're correct? Happy to look into this myself if you don't have time to add a test, but I think one should be added if possible. |
Actually you can probably do this by saving to svg (with rcParams["svg.fonttype"] = "none" to avoid converting glyphs to paths) (or pdf) and then looking for the substring in the result :p |
I've added a two line test that fails before this fix... I think the general argument of "it's not tested so there's no point adding tests" is a bit silly, and when we fix something as a rule there should always be a test added to make sure it doesn't break in the future. |
Thanks! |
Test by manually running the sankey examples.
Closes #11974 (comment).
PR Summary
PR Checklist