Skip to content

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

Merged
merged 2 commits into from
Jan 21, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 18, 2019

Test by manually running the sankey examples.

Closes #11974 (comment).

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 18, 2019
@anntzer anntzer added this to the v3.1 milestone Jan 18, 2019
Test by manually running the sankey examples.
@dstansby
Copy link
Member

I'm confused as to what issue this is solving; could you add a simple example to the test suite that raises the error?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 20, 2019

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.
As I already explained elsewhere, the sankey module is nearly entirely untested, and while it would be nice to improve the situation, that's not the point of this PR.

@dstansby
Copy link
Member

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 ValueError here doesn't seem like a huge job to me, and improving the test coverage bit by bit is a good thing.

Copy link
Member

@dstansby dstansby left a 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

@anntzer
Copy link
Contributor Author

anntzer commented Jan 20, 2019

But the ValueError is not actually the relevant point; what matters is the fact that if labels is a list of n strings, then the previous version of the code would create a list containing n copies of labels ([labels] * n), whereas the current version doesn't.
IOW, testing for the ValueError doesn't actually check the problem.

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.

@dstansby
Copy link
Member

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.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 20, 2019

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

@dstansby
Copy link
Member

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.

@dstansby dstansby dismissed their stale review January 21, 2019 12:43

Test added

@anntzer
Copy link
Contributor Author

anntzer commented Jan 21, 2019

Thanks!

@QuLogic QuLogic merged commit 4e967f3 into matplotlib:master Jan 21, 2019
@anntzer anntzer deleted the sankey-labels branch January 22, 2019 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants