Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add support for multiple hatches, edgecolors and linewidths in histograms#28073
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
Conversation
I'd suggest showing what this does with an example either in the GitHub pr description, or ideally in the gallery |
I'm not really sure if I need to add a new test or just modify an exisiting one(test_hist_stacked_bar) in test_axes.py |
I guess there are two things here:
|
pinging@story645 for review. The failing tests are unrelated |
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.
also needs updated documentation that the patch properties are now vectorized (@timhoffm any concerns here?)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
timhoffm commentedMay 19, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What should we be concerned about? We already have vectorized
This should also get a what's new entry. |
My bias is vectorize everything so I don't have concerns, but in the past for some vectorization discussions there have been concerns about the tradeoffs. But if there isn't opposition, awesome! |
I don't see any drawbacks for |
So we are planning to vectorizeall parameters of Patches? Like joinstyle, capstyle etc. |
Uh oh!
There was an error while loading.Please reload this page.
Not at this time w/ the current architecture, especially because nobody has asked for those. |
150c63b
to2351cbd
CompareCodecov is acting fishy, it passed once and failed again after squashing. Anything else to add/change? |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Impaler343 commentedJun 5, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
As of the last commit, this behavior is consistent for all histtypes unless mentioned otherwise:
|
story645 commentedJun 6, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Impaler343 I started trying to flow chart that and then realized it's probably clearer as a table. Let me know if this jives with what you're saying:
|
Have bolded the corrected ones |
I'm unable to fix CircleCI errors for docs. Could someone help me out? |
Hi, so the error is in your what's new: /home/circleci/project/doc/users/next_whats_new/histogram_vectorized_parameters.rst:47: WARNING: Explicit markup ends without a blank line; unexpected unindent. error:https://app.circleci.com/pipelines/github/matplotlib/matplotlib/31532/workflows/2533c013-7833-4d0c-ae72-dead7c7fbc76/jobs/83481?invite=true#step-113-207133_109 |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Impaler343 commentedJun 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Ok it turns out the vectorization of
|
story645 commentedJun 9, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think this input is ambiguous cause it can mean either:
So I think it's ok to error out (unless/until someone comes around expecting 1 or 2) but we should maybe special case and have a better error message, like if not all(colors) (if any are NaN) "Ambiguous color specification: colors in the list may not be None" Or something like that. |
The added lines have all been covered, codecov seems to still fail |
lib/matplotlib/tests/test_axes.py Outdated
@@ -4614,6 +4672,10 @@ def test_hist_emptydata(): | |||
ax.hist([[],range(10),range(10)],histtype="step") | |||
deftest_hist_none_patch(): | |||
plt.hist([1,2],label=["First","Second"]) |
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.
plt.hist([1,2],label=["First","Second"]) | |
plt.hist([],label=["First","Second"]) |
None patch is the empty list/array case
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.
And what you probably want to test is
assert len(patches) == 0
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 too works, basically the None condition is hit when we don't have enough datasets to cover each label. This is the doing ofitertools.zip_longest
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.
Should i still change it up? As the previous one seems clearer
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.
Sorry, I thought that zip_longest would copy the patch to both labels. I think there needs to be some sort of assert that you're test is doing what you think it is - maybe then checking that len(patches) != len(labels)
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.
What is len(lbs) here?
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.
The number of labels that have been used in the plot
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.
Sorry, I meant what were the actual numbers/does this test pass.
But yes if this test passes and coverage improves (add a comment that this test is to cover the if not patch case), then I'll let this concern go.
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.
But also assert len(patches) < len(labels) too I think?
Impaler343Jul 1, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Yes the tests pass, and yes we can add the length(patches) < len(labels) one as well, does the same comparison.
The actual numbers are 1 < 2
Cause I think this maybe got lost, I was suggesting that this construction makes the skip case clearer: forpatch,lblinitertools.zip_longest(patches,labels):ifnotpatch:continuep=patch[0]kwargs.update({'hatch':next(hatches),'linewidth':next(linewidths),'linestyle':next(linestyles),'edgecolor':next(edgecolors),'facecolor':next(facecolors), })p._internal_update(kwargs)iflblisnotNone:p.set_label(lbl)forpinpatch[1:]:p._internal_update(kwargs)p.set_label('_nolegend_') |
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.
Thanks for all your patience and extra thanks for tracing through and figuring out and testing our histogram color semantics/precedence 😅
I think the plot tests are now at a state where it's easy to see which parameter failed but there's also now only six image tests.
I'm pretty sure that the color semantics test doesn't try to draw anything but there might be a "build only artist tree" function that I'm missing.
lib/matplotlib/tests/test_axes.py Outdated
deftest_hist_none_patch(): | ||
# To cover None patches when excess labels are provided | ||
labels= ["First","Second"] | ||
patches= [[1,2]] | ||
fig,ax=plt.subplots() | ||
ax.hist(patches,label=labels) | ||
_,lbls=ax.get_legend_handles_labels() | ||
assert (len(lbls)<len(labels)andlen(patches)<len(labels)) |
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.
Can you please elaborate what this tests? What are None patches? Does this express what you want to test?
deftest_hist_none_patch(): | |
# To cover None patches when excess labels are provided | |
labels= ["First","Second"] | |
patches= [[1,2]] | |
fig,ax=plt.subplots() | |
ax.hist(patches,label=labels) | |
_,lbls=ax.get_legend_handles_labels() | |
assert (len(lbls)<len(labels)andlen(patches)<len(labels)) | |
deftest_hist_unused_labels(): | |
# When a list with one dataset and N elements is provided and N labels, ensure | |
# hat the first label is used for the dataset and all other labels are ignored | |
fig,ax=plt.subplots() | |
ax.hist([[1,2,3]],label=["values","unused","also unused"]) | |
_,labels=ax.get_legend_handles_labels() | |
assertlabels== ["values"] |
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.
None patches are the ones generated when the number of labels specified are more than the number of patches(Filled in by zip_longest). The suggested test also covers the line and has clearer labels, ill change it up
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
#28533 reminded me that this should get a versionadded directive https://matplotlib.org/devdocs/devel/api_changes.html#announce-new-and-deprecated-api |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
The doc build failure could be fixed by rebasing on |
ed56f95
intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
I'm so glad that this finally got merged! Thank you for the timely reviews |
Thanks for keeping up with all the reviews! |
- [Hatch filled histogram](https://matplotlib.org/stable/gallery/lines_bars_and_markers/filled_step.html) Hatching in histograms is fully supported throughmatplotlib#28073. This is [now simple](https://matplotlib.org/devdocs/gallery/statistics/histogram_multihist.html#hatch) and does warrant a dedicated example.- [Percentiles in horizontal bar charts](https://matplotlib.org/stable/gallery/statistics/barchart_demo.html) This is a very complex example. But in the end, it's just a bar plot with annotations. While it is statistics-themed through the use of perecntiles, the plot itself has no statistics-related characteristics.
- [Hatch filled histogram](https://matplotlib.org/stable/gallery/lines_bars_and_markers/filled_step.html) Hatching in histograms is fully supported throughmatplotlib#28073. This is [now simple](https://matplotlib.org/devdocs/gallery/statistics/histogram_multihist.html#hatch) and does warrant a dedicated example.- [Percentiles in horizontal bar charts](https://matplotlib.org/stable/gallery/statistics/barchart_demo.html) This is a very complex example. But in the end, it's just a bar plot with annotations. While it is statistics-themed through the use of perecntiles, the plot itself has no statistics-related characteristics.
Uh oh!
There was an error while loading.Please reload this page.
PR summary
Closes#26718 Distributes keyword args passed to each Patch using a cycler. Probably not the best way to do this?
PR checklist