Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
QuLogic merged 13 commits intomatplotlib:mainfromImpaler343:histogram
Jul 20, 2024

Conversation

Impaler343
Copy link
Contributor

@Impaler343Impaler343 commentedApr 13, 2024
edited
Loading

PR summary

Closes#26718 Distributes keyword args passed to each Patch using a cycler. Probably not the best way to do this?

PR checklist

@jklymak
Copy link
Member

I'd suggest showing what this does with an example either in the GitHub pr description, or ideally in the gallery

Impaler343 reacted with thumbs up emoji

@github-actionsgithub-actionsbot added the Documentation: examplesfiles in galleries/examples labelApr 14, 2024
@Impaler343
Copy link
ContributorAuthor

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

@oscargus
Copy link
Member

I guess there are two things here:

  1. Add/modify an example in the gallery to illustrate how it is used. (Will be easier for the reviewers to get an idea of how it is used etc.)
  2. Add a test to get the code coverage up and make sure no one breaks it later. Maybe there is some old example that one can "hi-jack", but otherwise create a new test with all the bells and whistles turned on.
Impaler343 reacted with thumbs up emoji

@Impaler343
Copy link
ContributorAuthor

pinging@story645 for review. The failing tests are unrelated

Copy link
Member

@story645story645 left a 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?)

@timhoffm
Copy link
Member

timhoffm commentedMay 19, 2024
edited
Loading

also needs updated documentation that the patch properties are now vectorized (@timhoffm any concerns here?)

What should we be concerned about? We already have vectorizedlabel andcolor. It's only fair to vectorize these insidehist() as well. This should be documented with**kwargs (can't immediately suggest, because that part has not been touched in the PR). Something like:

        **kwargs            `~matplotlib.patches.Patch` properties. The following properties additionally            accept lists of property values, one element for each dataset:            *edgecolors*, *linewidths*, *linestyles*, *hatches*.

This should also get a what's new entry.

Impaler343 reacted with thumbs up emoji

@story645
Copy link
Member

What should we be concerned about? We already have vectorized label and color.

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!

@timhoffm
Copy link
Member

I don't see any drawbacks forhist()

@Impaler343
Copy link
ContributorAuthor

So we are planning to vectorizeall parameters of Patches? Like joinstyle, capstyle etc.

@story645
Copy link
Member

So we are planning to vectorize all parameters of Patches? Like joinstyle, capstyle etc

Not at this time w/ the current architecture, especially because nobody has asked for those.

Impaler343 and timhoffm reacted with thumbs up emoji

@Impaler343Impaler343force-pushed thehistogram branch 2 times, most recently from150c63b to2351cbdCompareMay 29, 2024 07:34
@Impaler343
Copy link
ContributorAuthor

Codecov is acting fishy, it passed once and failed again after squashing. Anything else to add/change?

@Impaler343
Copy link
ContributorAuthor

Impaler343 commentedJun 5, 2024
edited
Loading

As of the last commit, this behavior is consistent for all histtypes unless mentioned otherwise:

  • Ifedgecolor, facecolor and color are mentioned, color is completely ignored.
  • Ifedgecolor and color are mentioned, color is applied to only the faces and edgecolor to the edges.
  • Iffacecolor and color are mentioned, color is applied to only the edges and facecolor to the faces.
  • Iffacecolor and edgecolor are mentioned, edgecolor is applied to edges, and facecolor is applied to faces.
  • Ifonly color is mentioned, the face is colored for all filled histograms, and only the edge is colored for unfilled histograms with the values incolor.
  • Ifonly facecolor is mentioned, the face is colored for all filled histograms with the value infacecolor, and the edges are colored with the default color cycle for unfilled histograms.
  • Ifonly edgecolor is mentioned, the face is colored by the default color cycle for all filled histograms, and the edge is colored with the values inedgecolor for all histograms.
    I feel this is the required behavior, but I am unsure if the color setting for the kwargs can be written more concisely.
    Let me know if I should start writing tests based on this structure.

@story645
Copy link
Member

story645 commentedJun 6, 2024
edited
Loading

@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:

hist typecolorfacecoloredgecolorpatch face colorpatch edge color
allsetsetsetfacecoloredgecolor
allsetNonesetcoloredgecolor
allsetsetNonefacecolorcolor
allNonesetsetfacecoloredgecolor
bar, barstacked, stepfilledsetNoneNonecolor'none'
stepsetNoneNone'none'color
bar, barstacked, stepfilledNonesetNonecolor'none'
stepNonesetNone'none'default color cycle
bar, barstacked, stepfilledNoneNonesetdefault color cycle'none'
stepNoneNoneset'none'edgecolor
bar, barstacked, stepfilledNoneNoneNone
stepNoneNoneNone

@Impaler343
Copy link
ContributorAuthor

hist typecolorfacecoloredgecolorpatch face colorpatch edge color
allsetsetsetfacecoloredgecolor
allsetNonesetcoloredgecolor
allsetsetNonefacecolorcolor
allNonesetsetfacecoloredgecolor
bar, barstacked, stepfilledsetNoneNonecolor'none'
stepsetNoneNone'none'color
bar, barstacked, stepfilledNonesetNonefacecolor'none'
stepNonesetNone'none'default color cycle
bar, barstacked, stepfilledNoneNonesetdefault color cycleedgecolor
stepNoneNoneset'none'edgecolor
bar, barstacked, stepfilledNoneNoneNonedefault color cycle'none'
stepNoneNoneNone'none'default color cycle

Have bolded the corrected ones

story645 and timhoffm reacted with thumbs up emoji

@Impaler343
Copy link
ContributorAuthor

I'm unable to fix CircleCI errors for docs. Could someone help me out?

@story645
Copy link
Member

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.
/home/circleci/project/doc/users/next_whats_new/histogram_vectorized_parameters.rst:51: ERROR: Unexpected indentation.
/home/circleci/project/doc/users/next_whats_new/histogram_vectorized_parameters.rst:64: WARNING: Explicit markup ends without a blank line; unexpected unindent.
/home/circleci/project/doc/users/next_whats_new/histogram_vectorized_parameters.rst:68: ERROR: Unexpected indentation.

error:https://app.circleci.com/pipelines/github/matplotlib/matplotlib/31532/workflows/2533c013-7833-4d0c-ae72-dead7c7fbc76/jobs/83481?invite=true#step-113-207133_109
rendering:https://output.circle-artifacts.com/output/job/2c962d26-1c66-49dd-a987-e5bf25c8fcbe/artifacts/0/doc/build/html/users/next_whats_new/histogram_vectorized_parameters.html

@Impaler343
Copy link
ContributorAuthor

Impaler343 commentedJun 8, 2024
edited
Loading

Ok it turns out the vectorization ofcolors is somewhat incomplete:

  • When we pass None, calls the color cycle, which is fine.
  • When we pass a list of valid colors, it uses them, which is also fine.
  • When we pass alist of Nones or even if one value in the list is None, it throws an error saying invalid RBGA argument. What can we do in this situation? Should we say that we use the colour cycle even if one value in the list is None and raise a warning? Or call the color cycle's next color whenever a None is encountered in a list? Or simply assume that the user will not pass a list like color= [None, "green", None] and ignore this check?

@story645
Copy link
Member

story645 commentedJun 9, 2024
edited
Loading

or simply assume that the user will not pass a list like color= [None, "green", None]

I think this input is ambiguous cause it can mean either:

  1. opt those two lines into the color cycle such that it maps to ['C0', "green", 'C1']
  2. set both lines to the same default ['C0', "green", 'C0']

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.

Impaler343 reacted with thumbs up emoji

@Impaler343
Copy link
ContributorAuthor

The added lines have all been covered, codecov seems to still fail

@@ -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"])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
plt.hist([1,2],label=["First","Second"])
plt.hist([],label=["First","Second"])

None patch is the empty list/array case

Copy link
Member

@story645story645Jun 28, 2024
edited
Loading

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

Copy link
ContributorAuthor

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

Copy link
ContributorAuthor

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

Copy link
Member

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)

Copy link
Member

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?

Copy link
ContributorAuthor

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

Copy link
Member

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.

Copy link
Member

@story645story645Jul 1, 2024
edited
Loading

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?

Copy link
ContributorAuthor

@Impaler343Impaler343Jul 1, 2024
edited
Loading

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

story645 reacted with thumbs up emoji
@story645
Copy link
Member

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_')
Impaler343 reacted with thumbs up emoji

Copy link
Member

@story645story645 left a 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.

Impaler343 reacted with heart emoji
Comment on lines 4675 to 4682
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))
Copy link
Member

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?

Suggested change
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"]

Copy link
ContributorAuthor

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

@story645
Copy link
Member

#28533 reminded me that this should get a versionadded directive

https://matplotlib.org/devdocs/devel/api_changes.html#announce-new-and-deprecated-api

@QuLogic
Copy link
Member

The doc build failure could be fixed by rebasing onmain to get fixed CircleCI, but I built it locally and confirmed it builds, so I'm not going to wait for that, and just merge.

@QuLogicQuLogic merged commited56f95 intomatplotlib:mainJul 20, 2024
36 of 37 checks passed
@Impaler343
Copy link
ContributorAuthor

I'm so glad that this finally got merged! Thank you for the timely reviews

rcomer and story645 reacted with hooray emoji

@QuLogic
Copy link
Member

Thanks for keeping up with all the reviews!

timhoffm added a commit to timhoffm/matplotlib that referenced this pull requestAug 2, 2024
- [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.
timhoffm added a commit to timhoffm/matplotlib that referenced this pull requestAug 3, 2024
- [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.
@Impaler343Impaler343 deleted the histogram branchAugust 8, 2024 17:59
@Impaler343Impaler343 restored the histogram branchAugust 8, 2024 17:59
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@story645story645story645 approved these changes

@timhoffmtimhoffmtimhoffm left review comments

@rcomerrcomerrcomer left review comments

@QuLogicQuLogicQuLogic approved these changes

Assignees
No one assigned
Labels
Documentation: examplesfiles in galleries/examplesNew feature
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

[Bug]: stacked histogram does not properly handle edgecolor and hatches
7 participants
@Impaler343@jklymak@oscargus@timhoffm@story645@QuLogic@rcomer

[8]ページ先頭

©2009-2025 Movatter.jp