Skip to content
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

Wrong default display for mplhep.histplot #98

Closed
HDembinski opened this issue Mar 6, 2020 · 18 comments · Fixed by #99
Closed

Wrong default display for mplhep.histplot #98

HDembinski opened this issue Mar 6, 2020 · 18 comments · Fixed by #99

Comments

@HDembinski
Copy link
Member

HDembinski commented Mar 6, 2020

The command
hep.histplot(h, bins, ax=axs[0])
does not produce a histogram in HEP-style. It draws markers at the edges, which is wrong, and the lines of the histogram do not go down to zero.

hep.histplot(h, bins, edges=True, ax=axs[1])
is closer to the expected skyline, but the markers are still wrong. Apart from the wrong markers, this should be the default. In fact, I cannot imagine that one ever wants to have edges=False.

@eduardo-rodrigues
Copy link
Member

I agree with this - what is "default" now is not what we in HEP take it as default. The whole point of mplhep is to provide a simple, handy and HEP-like interface atop matplotlib, right?

@andrzejnovak, shall we try and take in community input to improve the package? You should feel more than free to discuss APIs and anything relevant on the Gitter channel, to converge at best. To me this suggestion makes sense and should be taken on board asap.

Thanks.

@andrzejnovak
Copy link
Member

@HDembinski I am not 100% sure what you mean about the markers.

Re edges, there was a discussion somewhere in matplotlib/matplotlib#6669, https://docs.google.com/document/d/1ju3JObrPEyT7QpCx3m1YoVp0gFbLay8ZIxtO0mUCvgo/edit# and the gitter about this and one point that resonated with me is that drawing the edges seems to make a statement about the distribution end which is usually not the case.

I'm open to changing it but would like a better reason than that's how ROOT does it.

@eduardo-rodrigues
Copy link
Member

Apologies for chiming in again. In the end the edges are just reflecting the binning structure and the contents per bin, no? So, once you make a binned plot the edges just fall to zero if no entries are in the bin(s) after. Seems natural to me. Nothing to do with what ROOT does ;-).

@andrzejnovak
Copy link
Member

From matplotlib/matplotlib#6669

"Note that it does not cause the line to go to zero at the end, as we want--we have no information about this empirical PDF outside the specified binning."

In the proposed setup the edge does not bring additional information to the plot, but if you interpret it as such it can send the wrong message.

@HDembinski
Copy link
Member Author

HDembinski commented Mar 9, 2020

Full agreement with Eduardo. Also, when you fill the histogram with solid color, you also see the first and last edge. The natural expectation is that a non-filled histogram shows the outline of the filled region, hence also the first and last edge.

@HDembinski
Copy link
Member Author

HDembinski commented Mar 9, 2020

Regarding the markers, this is how "Examples.ipynb" looks on my computer when I checkout and install the master branch:

grafik

The markers have to go.

@eduardo-rodrigues
Copy link
Member

On your posting

"Note that it does not cause the line to go to zero at the end, as we want--we have no information about this empirical PDF outside the specified binning."
: this is not relevant for 99% of histograms one produce, where you're not doing a likelihood fit with a model containing several PDFs. I mean, most histograms we plot have nothing to do with PDFs. It's just that you literally plot the data and empty bins are just that. Also, the end of a histogram shows there is no data/MC data beyond that point or in the next bin(s).

Hans's comment on filled histograms are ever more clear/strong.

@eduardo-rodrigues
Copy link
Member

Feel free to bring in others to the conversation, BTW ...

@henryiii
Copy link
Member

henryiii commented Mar 9, 2020

Copying my comment from the IRIS-HEP Slack, histograms are really outlines of a bar plot, we just generally remove the inner lines for clarity. Removing the outer lines does not work, it ruins the outline.

@andrzejnovak
Copy link
Member

andrzejnovak commented Mar 9, 2020

@HDembinski you must be doing something to your rcParams. The whole plot looks neither like the mpl default or any of the mplhep styles.

Otherwise the markers (not usually visible) are a result of the the implementation of the underlying mpl function.

I could probably force set them in the function, but that's not great.

@henryiii
Copy link
Member

henryiii commented Mar 9, 2020

Can I recommend a few example notebooks, integrated into docs using nbsphinx, that are run automatically? It would be great for seeing examples of what the plots and styles look like. @andrzejnovak I can help you set it up if you want me do. See Boost-histogram's example section.

@henryiii
Copy link
Member

henryiii commented Mar 9, 2020

There's also JupyterBook, if you don't plan to have Sphinx for any of the other docs. Example (not mine): https://cranmer.github.io/madminer-tutorial/intro

@andrzejnovak
Copy link
Member

@henryiii Auto built docs are on the to do, but it was taking me longer than i thought

@henryiii
Copy link
Member

henryiii commented Mar 9, 2020

I could probably force set them in the function, but that's not great.

Unless the markers get drawn at every corner (Mickey-mouse style), they really are not even correct and would probably not ever be useful and could be forced off, I would think.

@andrzejnovak
Copy link
Member

As i said, i am willing to change default, but i am surprised how strongly people feel about this, given it's entirely cosmetic and doesn't carry any more information than without edges.

I do suspect this is more than anything to do with just being used to it looking the way it does in ROOT.

@henryiii
Copy link
Member

henryiii commented Mar 9, 2020

JuptyerBooks are really easy (I converted this class in a couple of hours of meetings), though I haven't built one into GHA yet (on my do list), I can send you a ping whenever I do that if you think that is a way you might want to go. If you are interested in the Sphinx route, then boost-histogram is an example that might be useful (I have auto building off there, since the C++ parts are not built on ReadTheDocs).

@HDembinski
Copy link
Member Author

@andrzejnovak I am not doing anything special in my rcParams, and anyway, the markers should never be drawn whatever I have in my rcParams.

@andrzejnovak
Copy link
Member

Fair enough, I can't imagine a use case where one would actually want to show those markers, so I think we can hard-code it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants