Allow for irregular bin widths in hist.plot.plot_pull_array

[meliache]

Assuming the axis has an edges attribute, I assume it should also have a widths attribute, so I thought we can just use that for the pull bar widths instead of calculating our own assuming a regular spacing (which might not be true).

For the patch-width, we just use right_edge - left_edge, instead of what was done before, which dividing this difference by the number of pulls and then multiplying them again with the same number.

From git-blame it looks like @matthewfeickert had written the original function so I might mention him.

I should also mention I'm not using this function, I just looked at it because I was writing my own pull plot function and was considering adapting the code for the rectangular patches and then just wondered why hist.axes[0].widths isn't used. Thus, it would be good if this could be tested properly.

• Find why pulls in ratio_plot with variable binning are larger than their errorbars and fix this (see this plot)

[meliache]

Now I looked at the file again and plot_ratio_array also uses a constant bar width assumption:

hist/src/hist/plot.py

Line 428 in 06a4859

bar_width = (right_edge - left_edge) / len(ratio)

I didn't use that function and am not sure how it's used and if possibly the ratio doesn't make sense with irregular binnings 🤷 But if you say it's safe I might also change that. I noticed that when I saw the needs changelog label and thought I should consider which other functions would be affected by this and was thinking whether plot_ratio_array would now also allow for non-regular axes.

[henryiii]

CC @matthewfeickert, IIRC, on that one?

[matthewfeickert]

I'll try to take a look at this before EOD on Monday.

[meliache]

Thanks. Before merging I would also update the changelog, I was just waiting for feedback to my above comment, depending on which I might add further changes.

[henryiii]

That's what I'm waiting on feedback for, @matthewfeickert added it, IIRC. I think the answer is yes, that these were both added without realizing that axes had edges and such. Even if the calculation doesn't handle irregular bin widths, I still think it's not "wrong" to use the correct edges attributes to compute it (it would just need a check and error in that case).

[henryiii]

I'll be trying to get a release out today, FYI.

[matthewfeickert]

I'll be trying to get a release out today, FYI.

Once I'm out of my meeting I'll go through this before lunch.

[matthewfeickert]

Thanks for the PR @meliache and sorry for letting it get buried in notifications.

Now I looked at the file again and plot_ratio_array also uses a constant bar width assumption:

hist/src/hist/plot.py

Line 428 in 06a4859

bar_width = (right_edge - left_edge) / len(ratio)

I didn't use that function and am not sure how it's used and if possibly the ratio doesn't make sense with irregular binnings 🤷 But if you say it's safe I might also change that. I noticed that when I saw the needs changelog label and thought I should consider which other functions would be affected by this and was thinking whether plot_ratio_array would now also allow for non-regular axes.

Yeah, I think that's fine. In PR #161 I was indeed making the assumption that the bin widths are constant.

Even if the calculation doesn't handle irregular bin widths, I still think it's not "wrong" to use the correct edges attributes to compute it (it would just need a check and error in that case).

Nothing should explicitly depend on this, so I would agree that this can be propagated everywhere.

Beyond using the suggested behavior of your PR everywhere, can you also add a test that uses irregular binning?

I'll be trying to get a release out today, FYI.

@henryiii As I was slow and it is already evening for @meliache I doubt this would be ready to go into a new release today (I have other work that I need to get done before this afternoon as well). I can help @meliache with finishing this tonight or tomorrow if they'd like it, so given the "Hist Serialization and Interactivity" fellow project that you're going to be mentoring will probably have some new releases coming out soon what do you think about getting this PR into a following release, so that I don't slow you down here?

[henryiii]

Following release would be fine. Maybe even this one, since scikit-hep/boost-histogram#708 is a serious regression and hist will have to wait till after bh gets fixed. 😞

[matthewfeickert]

Following release would be fine. Maybe even this one, since scikit-hep/boost-histogram#708 is a serious regression and hist will have to wait till after bh gets fixed.

Ah very sorry to hear it. :( Hopefully, while serious, it isn't too much of a pain to patch. But sounds good RE: either this release or the next.

@meliache I like your plan, and if you'd like me to help add contributions if you're currently busy I'm happy to do so given that I delayed you here. Of course I'm happy to answer any questions you have and let you finish this one off yourself. 👍

[meliache]

@henryiii As I was slow and it is already evening for @meliache I doubt this would be ready to go into a new release today (I have other work that I need to get done before this afternoon as well). I can help @meliache with finishing this tonight or tomorrow if they'd like it

Thanks for considering my time zone, evening is good as I don't have any meeting or other work to do, so I can add a couple of things like adapting plot_ratio_array and adding a changelog entry and possibly a test (see below), but after this I'll be a way from my keyboard and probably unresponsive. I never considered any release dates. So, if I'm unresponsive after 21 CET and you want this to be merged before tomorrow, feel free add what you think might be missing.

Beyond using the suggested behaviour of your PR everywhere, can you also add a test that uses irregular binning?

Good idea, a test would make me more comfortable merging this. The question is what to test. I would assume

1. Test that the changed plotting functions run without exceptions with regular as well as irregular bins
2. Test that we get the expected widths, e.g. how the resulting plots look like. For that I would use a regular axis or a variable axis with a regular binning and check that the bin widths are what we would expect from the constant-width calculation used prior to this PR. I could either look at the matplotlib artists that the plotting functions return, or I could use something like unittest.mock to mock the matplotlib plotting functions and check which arguments they were called with. Tbh the latter seems simpler to me. Update: Oh, I see you actually do image comparisons in tests, never saw that but it's nice.

I just took a look at your tests directory, probably it should go into test_plot.py, either into test_general_plot_pull or a separate similar test. The

[henryiii]

You can use nox -s regenerate to make/update test images (linux or docker). In 2-3 hours, boost-histogram release will complete, and I'll start working on releasing Hist.

[meliache]

I just realized that _fit_callable_to_hist also doesn't take into account varying bin widths when calling _curve_fit_wrapper. Instead of histogram.values() they should be somehow normalize by the widths, e.g. like what the binwnorm parameter does in the plotting functions. This is one of the things we should fix before claiming that h.plot_pull allows for variable axes.

Update: Untested hotfix in 37b6d3d

[henryiii]

$ docker run --rm -v$PWD:/hist -w/hist -it quay.io/pypa/manylinux2014_x86_64
# pipx run nox -s regenerate

It's easier to compare online, so I'm just pushing the result.

[henryiii]

(Sorry for the delay pushing, got nerd-sniped) Looks like this messes with the scaling - should this be normalized by the total width?

[meliache]

(Sorry for the delay pushing, got nerd-sniped) Looks like this messes with the scaling - should this be normalized by the total width?

Upps, I think I messed this up in 37b6d3d. I didn't test that and somehow assumed that it should be right at that time. Now I think it should work if I replace the np.sum in there with a np.mean, i.e.

    h_values_width_corrected = histogram.values() / (bin_widths / np.mean(bin_widths))

In that case, in case of a regular binning, the hist values remain unchanged, but if a bin is e.g. twice the mean bin width, the value will be divided by half. Instead of np.mean(bin_widths) we could also just do (right_edge - left_edge) / num_bins, but I doubt this will be much faster in practice.

Anyway, my local tests now fail again, but I think this is because you changed the baseline. Can I just revert that commit when I think I have fixed it?

In case your rushing to review this PR to get it into the new release, I'm not sure I can propagate this all the way up, since this affects more functions that I expected (at least if we want to allow variable axes in general) and I'm still not sure how to best tackle the tests and getting good coverage might need some time for me (though I'd be glad if @matthewfeickert could help with that).

Update: I see you already suggested a fix, thanks. And by the way, my mind was blown when I saw that github has image diffs.

[matthewfeickert]

...eventually making my way back to this after getting some other reviews done. It looks like tests are currently passing (:+1:) but

and I'm still not sure how to best tackle the tests

I can help add some example tests to cover the cases of irregular binning. I think(?) that hist doesn't do Codecov reporting or anything like that (@henryiii if you're game I could add Codecov), but from the diff there's basically nothing that would be uncovered (:+1:) and this is just testing functionality / sniffing for pathological examples / using tests as examples to future us.

[henryiii]

Sure, I'd be okay with Codecov. Is that the one that adds a comment to every PR? :/

[matthewfeickert]

Sure, I'd be okay with Codecov. Is that the one that adds a comment to every PR? :/

Yeah. It updates it in place each time there is a coverage change. c.f. scikit-hep/pyhf#1809 (comment)

Not sure if that's desirable or not in your mind.

[henryiii]

That's mildly annoying but not terrible. Though I'm guessing there's a way to avoid it: pypa/build#443 for example.

[meliache]

To be honest I also got busy with other things, sorry for not replying this week earlier, but I could revisit the tests.

I can help add some example tests to cover the cases of irregular binning.

Thanks a lot. I think I got overwhelmed by open questions how exactly to do the tests. E.g. should I include asserts in with variable binnings into the existing test functions or create new test functions? What should I best do to avoid code duplication, because necessarily I will basically repeat existing tests with non-regular binnigngs? How fine-grained should the tests be? Should I just test non-crashing behaviour or also generate example images and if yes, I'm not sure what the best procedure is for that. But as always don't let perfection be the enemy of the good

[meliache]

I just add two tests:

• test_image_plot_ratio_variable_axis_with_regular_bins: Same as test_image_plot_ratio_callable, but use an axis.Variablewith manually set regular bin edges, same as former test with axis.Regular, so the plot should look the same as the axis.Regular plot, thus I re-use the baseline here.

• test_image_plot_ratio_callable_bayesian_blocks_bins: Do a ration plots with variable bin widths. For this I ran the bayesian blocks algorithm on 5000 random numbers because I was too lazy to hand-pick variable bins.

Also I added a helper function to create these plot_ratio_callable test for different axes/binnings in order to avoid code duplication.

However, I see that for some python versions the plot diffs fail 🤔 . Also I saw that the new baseline for the variable binning looks off, the ratios are larger than the errorbars, so I think I have to adapt the errorbar or ratio calculation, but I decided to upload it anyway to at least have some tests as a starting point.
Update: Seems fixed now after f8bb460

[meliache]

Now I have arrived where I'm personally happy in this PR, I think I propagated my change up so that irregular bin widths should work for plot_pull and plot_ratio and I think the tests and the new baseline for plot_ratio with irregular bins look good now. I will no stop working on this for now and await further review. If you need more tests please tell me.

Coding-style wise I noticed that I now often repeat the same 2 to 3 lines to normalize the histogram values to the mean bin width. I think this could be factored out into a private helper function but for just 2 lines this doesn't really seam worth it, so I'm not sure. I'll leave it like it's now, except you think it's better to write a function for it.

[matthewfeickert]

Thanks. I'll take a look at this tonight after I've finished Snowmass papers. But CI is passing and there are tests now, so my guess is that things are probably 👍.

[meliache]

Btw, hist_values_uncert seems unbound if other is not a callable or string but a histogram. This is not something I introduced in this PR so if I'm not mistaken this might warrant a separate issue.

The question would be whether we just want to use np.sqrt(self.variances()) or in case other also has variances, if we want to statistically combine both somehow 🤷‍♂️