Skip to content

Fix stepfilled histogram polygon bottom perimeter #15783

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 3, 2019
Merged

Fix stepfilled histogram polygon bottom perimeter #15783

merged 2 commits into from
Dec 3, 2019

Conversation

sauerburger
Copy link
Member

@sauerburger sauerburger commented Nov 28, 2019 •
edited
Loading

PR Summary

The polygon generated by hist() when histtype='stepfilled' and bottom is not None contains an invalid starting point with a y-value of zero resulting in a diagonal line at the bottom of the first bin. This PR fixes the computation of the starting point and thus removes the drawing artifacts from the first bin.

Closes #15782.

Details

The y-values of the polygon perimeter are initialized with zeros. The number of points is such that the start (0) and endpoint (8) can/should be identical. The index assignment of the points is as indicated in the following sketch:

polygon

The endpoint (8) is set to the given bottom value.

The first point (0) is not modified in subsequent code unless stacked is True. (In that case, the starting point (0) is made equal to the next point (1) before assigning the new top outline.) In our case, this means the starting point (0) is still at zero.

Finally, in the case of stepfilled, the endpoint (8) (with the correct y-value) is truncated. (In the case of step the whole lower edge is truncated, as it should be since it's not closed.)

This PR assigns the y-value of the endpoint (8 or -1 in general) to the staring point (0) for every polygon. This works regardless of stacked=True/False and step/stepfilled.

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

@ImportanceOfBeingErnest
Copy link
Member

Thanks for the PR.
The issue is present since at least matplotlib 2, which indicates two things: (a) Not many people seem to use the bottom argument together with "step*", (b) this feature seems insufficiently tested.
Would you like to add a (few) test(s), such that it wouldn't break again in the future?

@ImportanceOfBeingErnest ImportanceOfBeingErnest added this to the v3.3.0 milestone Nov 28, 2019
@sauerburger
Copy link
Member Author

Yes, that's a good point, sure.

Would you recommend

  • to check the geometry of the returned patches, or
  • to perform image comparison tests?

@ImportanceOfBeingErnest
Copy link
Member

It should be possible to do this without image comparison test (which are expensive memory- and maintainance-wise) by comparing the returned patch (or its vertices) to the expected coordinates.

@sauerburger sauerburger force-pushed the fix-stepfilled-hist-bottom branch 2 times, most recently from a3a238f to e0854bd Compare November 28, 2019 22:01
@sauerburger
Copy link
Member Author

sauerburger commented Nov 28, 2019 •
edited
Loading

I've added a couple of test cases for different combinations of stacked, step/stepfilled and bottom. The tests

  • test_hist_stepfilled_bottom_geometry and
  • test_hist_step_bottom_geometry

fail on master and succeed after this fix.

There was already an image comparison test to check the outline when bottom is given, test_hist_step_bottom, but by chance the first point of the outline should be at (0, 0), so the test didn't catch this scenario.

@sauerburger sauerburger force-pushed the fix-stepfilled-hist-bottom branch from e0854bd to 0786768 Compare November 28, 2019 23:03
timhoffm
timhoffm reviewed Nov 28, 2019
View reviewed changes
Copy link
Member

@timhoffm timhoffm 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, you could also do without the blank lines in the first four examples. They are short and clear enough.

@sauerburger
Test geometry of step(filled) histograms
60a785d
@sauerburger
Fix stepfilled histogram polygon bottom perimeter
f40b0f8 
The polygon generated by hist() when histtype='stepfilled' and bottom is not
None contained an invalid starting point with a y-value of zero resulting in a
diagonal line at the bottom of the first bin. This commit fixes the computation
of the starting point and thus removes the drawing artifacts from the first bin.
@sauerburger sauerburger force-pushed the fix-stepfilled-hist-bottom branch from 0786768 to f40b0f8 Compare November 29, 2019 14:39
@anntzer
Copy link
Contributor

anntzer commented Dec 3, 2019

Thanks!

@anntzer anntzer merged commit 96152ad into matplotlib:master Dec 3, 2019
@anntzer anntzer mentioned this pull request Jun 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Invalid polygon in stepfilled histogram when bottom is set
4 participants
@sauerburger @ImportanceOfBeingErnest @anntzer @timhoffm