feat : implement under/overflow function #413

Ming-Yan · 2023-03-02T23:08:43Z

workable version for #337

implemented in histplot,hist2dplot function
"hint" : show text if under/overflow exist, also printout the value
"show": additional bin with 5 times larger bin width
"sum": sum under/overflow bin to first/last bin

andrzejnovak · 2023-03-02T23:34:07Z

Thanks a lot! I'll take a look tomorrow.

andrzejnovak · 2023-03-03T08:17:21Z

Alright. Can you split this into two PRs, I think the plot_ratio plot is for a separate discussion. For to flow bin options, it would be nice if you could post what they look like and add them as tests.

src/mplhep/plot.py

andrzejnovak · 2023-03-03T09:26:54Z

For tests you can take a look at the ones implemented here https://github.com/scikit-hep/mplhep/blob/master/tests/test_basic.py

Ming-Yan · 2023-03-06T16:09:05Z

Add 1d/2d test plot

alexander-held · 2023-03-07T11:19:58Z

Hi, thanks for working on this! The "default" option above draws a lot of attention on the outmost bins, but does not explain what the red color corresponds to. In my opinion there is no special visualization needed if flow content is not included: this is the only case where the plot actually does what it says on the axis.

The "show" and "sum" cases on the other hand are not doing this, they suggest the bin contents have a value of the observable within the range indicated by the bin edges (which is not necessarily true). I think it would be helpful to denote somehow that those are special. One way in which this could be accomplished is by breaking the axis in the middle of the flow (or sum) bin. In principle this would also require the axis labels to be updated accordingly. This might not generalize too well, and I don't know how to translate that to 2d.

Ming-Yan · 2023-03-07T14:39:16Z

Hi @alexander-held , thanks for the suggestion,

Hi, thanks for working on this! The "default" option above draws a lot of attention on the outmost bins, but does not explain what the red color corresponds to. In my opinion there is no special visualization needed if flow content is not included: this is the only case where the plot actually does what it says on the axis.

Indeed this is the "hint" option, where highlight the bins if under/overflow contents exist

The "show" and "sum" cases on the other hand are not doing this, they suggest the bin contents have a value of the observable within the range indicated by the bin edges (which is not necessarily true). I think it would be helpful to denote somehow that those are special. One way in which this could be accomplished is by breaking the axis in the middle of the flow (or sum) bin. In principle this would also require the axis labels to be updated accordingly. This might not generalize too well, and I don't know how to translate that to 2d.

To have break axis, do you mean the style like this?https://matplotlib.org/3.1.0/gallery/subplots_axes_and_figures/broken_axis.html
I agree with the point that the flow bins value not next to the bin edges, but I think we also see some plots using the current implementation. For the sum option, as it merges to the last/first bin, I think split axis is not required

alexander-held · 2023-03-07T15:25:02Z

Indeed this is the "hint" option, where highlight the bins if under/overflow contents exist

Thanks for clarifying, I misunderstood this then. If the default behavior is that the plots do not change at all then that sounds reasonable to me.

For the broken axis: yes, I was thinking of something like that. To be clear here: I don't think this is crucially needed, it was just meant as an idea. I know that in practice there are many plots that use e.g. the "sum" approach. This is not specific to the changes here, it is a more general comment. Having the bins end at a clearly defined value on the axis that does not actually correspond to the cutoff for the bin content is what I find personally misleading. It is even more misleading in the examples above where the axis continues to the left and the right of the histogram, suggesting additional empty bins (so really suggesting that all events are in [5, 15]). I am not opposed to this implementation here though, users are still free to use what they want of course.

andrzejnovak · 2023-03-07T16:11:09Z

Thanks a lot @Ming-Yan for adding the pics. I am +1 on the broken axis (particularly for the "hint" and "show" options).

In my opinion there is no special visualization needed if flow content is not included: this is the only case where the plot actually does what it says on the axis.

I am not sure I agree that ignoring the flow bin does actually "do what it says" because for most plots the xlims don't match the extent of the histogram, but include some edge whitespace, which would suggest there's nothing there, rather than "no info here" which could be well shown with the broken axis.

alexander-held · 2023-03-07T17:05:23Z

Agreed on the issue of xlim not matching the range of the histogram. I guess I'd always set the limits so that this cannot happen in practice.

Ming-Yan · 2023-03-07T18:53:41Z

Thanks for the discussion, I would try to implemented broken axis for "hint" and "show" option

NJManganelli · 2023-03-07T19:14:52Z

+1 for Broken axis to show flow bins, this seems like a really nice way to communicate it visually

Ming-Yan · 2023-03-16T11:45:48Z

Implemented broken axis signed(not using subplots)
Also edit the axis label for show option

andrzejnovak

Nice. I think this is definitely in the right direction. The added test baseline png is not updated and the existing test baseline pngs should not be changed in this PR (there's no visual difference, it's just that they're not bit by bit exact copies, so doing git commit -a is adding them on.

For the viz in 1D I think we don't need the markers on the top axis and I think it would look better if the "broken axis" markers didn't go through the bin edges, but were contained "within" the bin.

I think if the gap value in "show" is set to nan it should also help in the 2D plot, where right now it looks like a 0 value bin, as opposed to a "missing" one.

.pre-commit-config.yaml

andrzejnovak · 2023-03-18T12:51:10Z

src/mplhep/plot.py

@@ -168,9 +171,42 @@ def histplot(
    )

    # Cast to plottables
+


redundant line

Still present?

src/mplhep/plot.py

Ming-Yan · 2023-03-19T00:17:39Z

Thanks @andrzejnovak for reviewing the updates. The suggests implemented in the new commit with adding a few more tests

src/mplhep/plot.py

- add additional space for "show" option, change the tick labels - using slash for "hint", "show" options

- plot: remove None option, put warning for no flow bins histogram - plot: add flow bins option ROOT histogram - test : add two-side,one-side and no flow bins

Conflicts: src/mplhep/plot.py tests/baseline/test_hist2dplot_flow.png tests/baseline/test_histplot_flow.png tests/test_basic.py

Ming-Yan · 2023-04-08T14:12:41Z

Adapt two-side, one-side no overflow bins plot

- plot/test: remove None options - test: add dedicated test for uproot/hist with various flow scheme - test: add type check (numpy/hist with flow=False) [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

tests/test_basic.py

src/mplhep/plot.py

andrzejnovak · 2023-04-11T15:33:05Z

src/mplhep/plot.py

+    for i, h in enumerate(hists):
+        value, variance = h.values(), h.variances()
+        if not isinstance(h, Hist.hist.Hist) and not "uproot.models.TH" in str(type(h)):
+            print(f"Warning: {type(h)} is not allowed to get flow bins")


Hmm, is this triggered every time you pass just an array as a hist?

Seems you have this solved for 2D with and flow is not None

This should be triggered every time, otherwise the hist would not filled as flow set to None after 1 iteration

Is this really a good practice tho? I don't think I should see this warning if I all I do is call histplot([1,2,3,2,1]) which I think will happen here right?

Right, add and flow is not None and indeed if we have None option, we would keep the case plottables.append(Plottable(value, edges=final_bins, variances=variance))

src/mplhep/plot.py

andrzejnovak · 2023-04-11T15:34:33Z

src/mplhep/plot.py

+        trans = mpl.transforms.blended_transform_factory(ax.transData, ax.transAxes)
+        kwargs = dict(
+            marker=[(-0.5, -d), (0.5, d)],
+            markersize=15,


could the marker size be somehow derived in relation to figure size?

yes, it possible, I take height to change the size

ax_h = ax.bbox.height kwargs = dict( marker=[(-0.5, -d), (0.5, d)], markersize=ax_h*0.05, linestyle="none", color="k", mec="k", mew=1, clip_on=False, transform=trans, )

feat : minor fixes - plot: fix warning for histplot - plot: rename to conflict with hist - plot: change slash size - test: make tests independent

andrzejnovak · 2023-04-14T06:52:01Z

tests/baseline/test_histplot_flow.png

This doesn't look right for None

find a weird bug, not sure where it from, if I swap the order of "sum" and None then I get the correct version, but we do this independently...
hep.histplot(h, ax=axs[0], flow="hint") hep.histplot(h, ax=axs[1], flow="show") hep.histplot(h, ax=axs[2], flow=None) hep.histplot(h, ax=axs[3], flow="sum") axs[0].set_title("Default(hint)", fontsize=18) axs[1].set_title("Show", fontsize=18) axs[3].set_title("Sum", fontsize=18) axs[2].set_title("None", fontsize=18)

So I checked out the PR, it seems the code is modifying the input object at some point, that's why it makes a mess

Ah.. sorry I think it's the confusion of check under/overflow bin where the elif flow is None : continue should be added. And I don't think it changes the hist object(we didn't modify it anywhere in the code)

Consider this, the object should not be different

np.random.seed(0) entries = np.random.normal(10, 3, 400) h = hist.new.Reg(20, 5, 15, name="x", underflow=True, overflow=False).Weight().fill(entries) hc = copy.deepcopy(h) h.plot() h.plot(flow='sum') h.plot(flow='show') h.plot(flow=None) plt.close() h.values(flow=True) == hc.values(flow=True)

andrzejnovak · 2023-04-14T06:52:44Z

tests/baseline/test_histplot_hist_flow.png

The one-sided flow tests seem to misbehave as well

Hmmm.. I think the problem is you cannot check the flow bin is underflow or overflow from the stored hist... You would see there's a flow bins(no h.values(underflow=True) to check)

Hist(Regular(20, 5, 15, overflow=False, name='x'), storage=Weight()) # Sum: WeightedSum(value=359, variance=359) (WeightedSum(value=377, variance=377) with flow)

Right, would have to check if the object has h.axes[0].traits and if underflow and overflow are set in there.

ah ok, then i need to add additional check for under/overflow bins for hist case, I just simply check

underflow = underflow + h.values(flow=True)[0] overflow = overflow + h.values(flow=True)[-1]

src/mplhep/plot.py

tests/test_basic.py

- plot: fix hist under/overflow bins check - plot: tmpfix on hist value/variances - add hist to setup.py

for more information, see https://pre-commit.ci

andrzejnovak

There are still couple of explicit hist checks to be rid of in the code. Plus some flake8 errors https://results.pre-commit.ci/run/github/184555939/1681661620.370AvrXARZSbUqgBkBfvJQ

src/mplhep/plot.py

andrzejnovak · 2023-04-17T11:02:53Z

I am not sure what's tripping up the actions, but the problem happens because a test fails. Checking locally it seems the 2d flow test doesn't have an updated example

- test: update None for hist2dplot - plot: make copy for hist - update plot

- test: fix cbar min/max - plot: fix vmin/vmax for cbar - plot: fix 2d flow bins check - plot: fix sum option

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci fix:CI

andrzejnovak · 2023-04-18T09:09:43Z

Thanks a lot @Ming-Yan!

henryiii · 2023-05-12T13:08:19Z

This causes a massive printout in some cases. @HDembinski made the str of a histogram a text plot of a histogram, which makes this warning printed unreadable. A fix would be to use the repr instead.

See the PyHEP histogramming Gitter channels for details from the user report.

Also, this should be a warnings.warn, not a print. That way users have control over it, it gets printed nicely (with color in a notebook), it's testable, etc.

henryiii · 2023-05-12T13:09:59Z

src/mplhep/plot.py

+        and not h.axes[0].traits.overflow
+    ):
+        flow = None
+        print(f"Warning:  you don't have flow bins stored in {h}")


This should have been:

warnings.warn(f"You don't have flow bins stored in {h!r}")

henryiii · 2023-05-12T13:10:27Z

src/mplhep/plot.py

+        and flow is not None
+    ):
+        print(
+            f"Warning: {type(h)} is not allowed to get flow bins, flow bin option set to None"


This should also be a warnings.warn. There's a Ruff check you can enable to disallow all print statements in library code.

henryiii · 2023-05-15T22:07:06Z

Also, I don't understand the default. Calling plot on a flowless histogram should not produce a warning about missing flow bins; you should not have to pass flow=None to get it to shut up. Also, if it takes a text string, it should be possible to specify everything via a text string, and None should be the same as not specifying anything. That's the standard meaning of None, and the reason it's typed as "Optional".

Ming-Yan · 2023-05-16T07:09:05Z

Hi @henryiii , apologize for noticing the suggestions late and thanks for the fixes

andrzejnovak reviewed Mar 3, 2023

View reviewed changes

Ming-Yan mentioned this pull request Mar 6, 2023

feat: correctionlib(jsonpog-integration) implementation & fixes on actions cms-btv-pog/BTVNanoCommissioning#50

Merged

Ming-Yan changed the title ~~feat : add plotting features~~ feat : implement under/overflow function #337 Mar 6, 2023

Ming-Yan changed the title ~~feat : implement under/overflow function #337~~ feat : implement under/overflow function Mar 6, 2023

Ming-Yan force-pushed the master branch from 2d41bdc to 4208d43 Compare March 7, 2023 07:37

Ming-Yan force-pushed the master branch from 92218a2 to 2c3b83f Compare March 16, 2023 13:50

andrzejnovak reviewed Mar 18, 2023

View reviewed changes

Ming-Yan force-pushed the master branch from 3dfcd9d to 063f1d9 Compare March 19, 2023 00:15

Ming-Yan mentioned this pull request Mar 23, 2023

Integrated correctionlib developments cms-rwth/CoffeaRunner#17

Merged

andrzejnovak assigned Ming-Yan Apr 3, 2023

Ming-Yan force-pushed the master branch from 063f1d9 to 2c3b83f Compare April 3, 2023 09:43

andrzejnovak reviewed Apr 3, 2023

View reviewed changes

src/mplhep/plot.py Show resolved Hide resolved

Ming-Yan force-pushed the master branch 2 times, most recently from 637677e to 359928a Compare April 3, 2023 13:54

feat : implemented broken-axis signed

0b3152f

- add additional space for "show" option, change the tick labels - using slash for "hint", "show" options

Ming-Yan force-pushed the master branch from 359928a to 0b3152f Compare April 3, 2023 13:55

feat : implemented broken-axis signed

e0ab8c4

- add additional space for "show" option, change the tick labels - using slash for "hint", "show" options

Ming-Yan added 2 commits April 8, 2023 16:07

feat : set "hint" as default, add ROOT histogram for flow bins

3d1d041

- plot: remove None option, put warning for no flow bins histogram - plot: add flow bins option ROOT histogram - test : add two-side,one-side and no flow bins

Merge branch 'master' of github.com:Ming-Yan/mplhep

d21f64d

Conflicts: src/mplhep/plot.py tests/baseline/test_hist2dplot_flow.png tests/baseline/test_histplot_flow.png tests/test_basic.py

Ming-Yan force-pushed the master branch from 9505dda to aa3d5b7 Compare April 11, 2023 15:00

feat : add more tests

1994306

- plot/test: remove None options - test: add dedicated test for uproot/hist with various flow scheme - test: add type check (numpy/hist with flow=False) [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci

Ming-Yan force-pushed the master branch from aa3d5b7 to 1994306 Compare April 11, 2023 15:21

andrzejnovak requested changes Apr 11, 2023

View reviewed changes

f eat : m

2935646

feat : minor fixes - plot: fix warning for histplot - plot: rename to conflict with hist - plot: change slash size - test: make tests independent

andrzejnovak requested changes Apr 14, 2023

View reviewed changes

andrzejnovak reviewed Apr 14, 2023

View reviewed changes

src/mplhep/plot.py Outdated Show resolved Hide resolved

andrzejnovak reviewed Apr 14, 2023

View reviewed changes

tests/test_basic.py Show resolved Hide resolved

Ming-Yan and others added 3 commits April 14, 2023 15:13

fix : under/overflow bins

3a737ac

- plot: fix hist under/overflow bins check - plot: tmpfix on hist value/variances - add hist to setup.py

[pre-commit.ci] auto fixes from pre-commit.com hooks

e092c87

for more information, see https://pre-commit.ci

Merge branch 'master' into master

93636db

andrzejnovak requested changes Apr 16, 2023

View reviewed changes

src/mplhep/plot.py Outdated Show resolved Hide resolved

src/mplhep/plot.py Outdated Show resolved Hide resolved

src/mplhep/plot.py Outdated Show resolved Hide resolved

src/mplhep/plot.py Outdated Show resolved Hide resolved

Ming-Yan added 2 commits April 16, 2023 19:02

fix : remove hist

5dc33ed

Merge branch 'master' of github.com:Ming-Yan/mplhep

156eb80

Ming-Yan added 3 commits April 17, 2023 16:37

feat : add None in hist2dplot

2a0cd3d

- test: update None for hist2dplot - plot: make copy for hist - update plot

fix : hist2d overflow bins handling

574ba6d

- test: fix cbar min/max - plot: fix vmin/vmax for cbar - plot: fix 2d flow bins check - plot: fix sum option

fix: remove hasattr(h, plot1d)

a2c039b

[pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci fix:CI

Ming-Yan force-pushed the master branch from 25b188b to a2c039b Compare April 17, 2023 22:22

andrzejnovak merged commit d0cf7ce into scikit-hep:master Apr 18, 2023

henryiii reviewed May 12, 2023

View reviewed changes

henryiii mentioned this pull request May 15, 2023

fix: flow warning fix #426

Merged

feat : implement under/overflow function #413

feat : implement under/overflow function #413

Conversation

Ming-Yan commented Mar 2, 2023 • edited Loading

andrzejnovak commented Mar 2, 2023

andrzejnovak commented Mar 3, 2023

andrzejnovak commented Mar 3, 2023

Ming-Yan commented Mar 6, 2023

alexander-held commented Mar 7, 2023

Ming-Yan commented Mar 7, 2023

alexander-held commented Mar 7, 2023

andrzejnovak commented Mar 7, 2023

alexander-held commented Mar 7, 2023

Ming-Yan commented Mar 7, 2023

NJManganelli commented Mar 7, 2023

Ming-Yan commented Mar 16, 2023

andrzejnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ming-Yan commented Mar 19, 2023

Ming-Yan commented Apr 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ming-Yan Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrzejnovak left a comment

Choose a reason for hiding this comment

andrzejnovak commented Apr 17, 2023

andrzejnovak commented Apr 18, 2023

henryiii commented May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii commented May 15, 2023

Ming-Yan commented May 16, 2023 • edited Loading

Ming-Yan commented Mar 2, 2023 •

edited

Loading

Ming-Yan Apr 14, 2023 •

edited

Loading

henryiii commented May 12, 2023 •

edited

Loading

Ming-Yan commented May 16, 2023 •

edited

Loading