Skip to content

feat: StepPatch to take array as baseline #18511

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
timhoffm merged 2 commits into from
Oct 5, 2020
Merged

feat: StepPatch to take array as baseline #18511

timhoffm merged 2 commits into from
Oct 5, 2020

Conversation

@andrzejnovak
Copy link
Contributor

@andrzejnovak andrzejnovak commented Sep 17, 2020 •
edited
Loading

PR Summary

Follow up PR to #18275 enabling an array as a baseline, useful for plotting stacked histograms. Disabling edge around the StepPatch by default.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

QuLogic
QuLogic reviewed Sep 18, 2020
View reviewed changes
@andrzejnovak andrzejnovak changed the title feat: StepPatch to take array as baseline [DRAFT] feat: StepPatch to take array as baseline Sep 18, 2020
@andrzejnovak andrzejnovak force-pushed the baselinearr branch 2 times, most recently from 08af034 to 378ac3d Compare September 18, 2020 09:10
@andrzejnovak
Copy link
Contributor Author

andrzejnovak commented Sep 18, 2020

Actually it seems I misunderstood the sticky behaviour. When no edges are shown (aka baseline=None) setting the sticky to 0 does not actually affect the drawing at all.

@andrzejnovak andrzejnovak force-pushed the baselinearr branch 2 times, most recently from 7ef7931 to b367d82 Compare September 21, 2020 12:29
@andrzejnovak andrzejnovak changed the title [DRAFT] feat: StepPatch to take array as baseline feat: StepPatch to take array as baseline Sep 21, 2020
@QuLogic
Copy link
Member

QuLogic commented Sep 22, 2020

This force_sticky_edges property seems like it should be a separate PR, but also the setter is not correct, so I'm not sure if it's working the way you expect.

@andrzejnovak
Copy link
Contributor Author

andrzejnovak commented Sep 22, 2020

@QuLogic Alright, I'll save it for a follow up PR

@andrzejnovak
Copy link
Contributor Author

andrzejnovak commented Sep 22, 2020

I am not sure why the docs didn't run.

@andrzejnovak
Copy link
Contributor Author

andrzejnovak commented Sep 22, 2020

This is ready for review, tagging @jklymak and @timhoffm since they guided the original PR.

@andrzejnovak
Copy link
Contributor Author

andrzejnovak commented Sep 25, 2020

bump?

dopplershift
dopplershift previously requested changes Sep 25, 2020
View reviewed changes
timhoffm
timhoffm reviewed Sep 25, 2020
View reviewed changes
timhoffm
timhoffm reviewed Sep 27, 2020
View reviewed changes
@timhoffm timhoffm mentioned this pull request Sep 29, 2020
Merged
@andrzejnovak andrzejnovak force-pushed the baselinearr branch 2 times, most recently from 3634e6e to 088d672 Compare September 29, 2020 20:28
@andrzejnovak
Copy link
Contributor Author

andrzejnovak commented Sep 29, 2020

Alright, this is cleaned up and should be ready pending #18579

@jklymak jklymak requested a review from dopplershift October 1, 2020 21:38
jklymak
jklymak reviewed Oct 1, 2020
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thanks! Couple of small nits, and as always don't hesitate to ping if this doesn't move in the next couple of days

timhoffm
timhoffm reviewed Oct 1, 2020
View reviewed changes
@andrzejnovak andrzejnovak force-pushed the baselinearr branch 2 times, most recently from 1d20bb2 to dd8b071 Compare October 2, 2020 00:21
timhoffm
timhoffm reviewed Oct 2, 2020
View reviewed changes
timhoffm
timhoffm reviewed Oct 2, 2020
View reviewed changes
jklymak
jklymak approved these changes Oct 5, 2020
View reviewed changes
Copy link
Member

@jklymak jklymak 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 looks good. Should be squashed before merging....

@timhoffm timhoffm dismissed dopplershift’s stale review October 5, 2020 16:11

Changes implemented.

@timhoffm timhoffm merged commit 4735755 into matplotlib:master Oct 5, 2020
@timhoffm
Copy link
Member

timhoffm commented Oct 5, 2020

Thanks @andrzejnovak!

@QuLogic QuLogic added this to the v3.4.0 milestone Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QuLogic QuLogic QuLogic left review comments

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

@dopplershift dopplershift Awaiting requested review from dopplershift

Assignees

No one assigned

Labels

New feature

Projects

None yet

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

6 participants

@andrzejnovak @QuLogic @timhoffm @dopplershift @jklymak @dstansby