[Python][UHI] Extend TH1 equality operator to consider additional properties

[siliataider]

This Pull request:

Changes or fixes:

The equality operator for ROOT histograms now compares the following properties:

• Histogram type (including dimension).
• Histogram kind.
• Shape (number of bins per axis).
• Bin contents array.
• Sum-of-weights-squared array.
• Axes properties.

Checklist:

• tested changes locally
• updated the docs (if necessary)

This PR fixes #19038

[vepadulano]

LGTM, but let's wait for discussion with @guitargeek

[siliataider]

@guitargeek I did not add checks for properties related to the GUI. We should discuss if those are part of determining whether 2 ROOT histograms are equal or not

[github-actions]

Test Results

    20 files      20 suites   3d 16h 28m 52s ⏱️
 3 013 tests  3 012 ✅ 0 💤 1 ❌
58 672 runs  58 671 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit f13b675.

♻️ This comment has been updated with latest results.

[dpiparo]

A discussion might be needed. I personally lean towards not considering aspects such as colours or marker type part of the equality: they are not related to the statistical properties of the histos and in any case those will not be part of the new histograms...

[guitargeek]

What about underflow and overflow bins? Fair enough for the plotting properties, but I think the *flow values are part of the content. Right now, this is not considered, also with your PR.

See:

hist1 = ROOT.TH1D("hist1", "hist1", 1, -1., 1.)
hist1.Fill(2.)
print(hist1.GetBinContent(0))

hist2 = ROOT.TH1D("hist2", "hist2", 1, -1., 1.)
hist2.Fill(-2.)
print(hist2.GetBinContent(0))

print(hist1 == hist2)

It will give:

0.0
1.0
True

And what about the name, and the title? Can two histograms be considered equal if they represent a different quantity? What about fit results that are associated with the histogram (more of a stretch but still we should think about it)?

I think this new equality operator is not really well defined, not documented, and potentially dangerous because it changes the behavior in an unexpected way (before, it compared by pointer, for better or for worse). Also, according to Vincenzos comment, it's not even part of UHI. So why do we really need it? You say we "need it to integrate the UHI test suite", but when I remove it, the uhi-plotting and uhi-indexing pythonization tests still pass for me locally. So is it really needed?

I think we should keep this new equality operator only if there is a clear motivation from a user perspective, not only for some test integration. And if there is a clear motivation, I think the equality needs to be made even stricter and also documented.

[vepadulano]

I think we should keep this new equality operator only if there is a clear motivation from a user perspective,

While I see your point and I understand this new behaviour might lead to potential confusion for downstream users in the wild, let me stress that the pre-existing behaviour in master was simply wrong and a bug. If we decide against merging this PR, imagining that we find a way to accommodate the requirements for the UHI testing suite without using this equality operator, I will request that the equality operator is simply disabled for TH1 and derived classes, i.e. h1 == h2 should throw an exception.

[guitargeek]

Yes, okay then we're on the same page here!

I will request that the equality operator is simply disabled for TH1 and derived classes, i.e. h1 == h2 should throw an exception.

I think that would mean kicking out this pythonization:
https://github.com/root-project/root/blob/master/bindings/pyroot/pythonizations/src/TObjectPyz.cxx

Then we don't even need to throw an exception but it's simply not implemented.