Bug in conversion of TH2D with fSumW2 to boost-histogram (uproot4 0.1.2)

[HDembinski]

I found a bug in the conversion from a TH2D with fSumW2 to a boost-histogram. The variance gets incorrectly transposed. Demo code:

import ROOT
import uproot4

h = ROOT.TH2D("h", "", 2, 0, 1, 2, 0, 1)
h.SetBinContent(1, 1, 1)
h.SetBinContent(1, 2, 2)
h.SetBinContent(2, 1, 3)
h.SetBinContent(2, 2, 4)
h.SetBinError(1, 1, 1)
h.SetBinError(1, 2, 2)
h.SetBinError(2, 1, 3)
h.SetBinError(2, 2, 4)

f = ROOT.TFile.Open("foo.root", "recreate")
h.Write()
f.Close()

with uproot4.open("foo.root") as f:
    h = f["h"]

hb = h.to_boost()

val, err = h.values_errors()
print("values")
print(val)
print(hb.view(True).value)

print("errors")
print(err)
print(hb.view(True).variance ** 0.5)

Returns

values
[[0. 0. 0. 0.]
 [0. 1. 2. 0.]
 [0. 3. 4. 0.]
 [0. 0. 0. 0.]]
[[0. 0. 0. 0.]
 [0. 1. 2. 0.]
 [0. 3. 4. 0.]
 [0. 0. 0. 0.]]
errors
[[0. 0. 0. 0.]
 [0. 1. 2. 0.]
 [0. 3. 4. 0.]
 [0. 0. 0. 0.]]
[[0. 0. 0. 0.]
 [0. 1. 3. 0.]
 [0. 2. 4. 0.]
 [0. 0. 0. 0.]]

The values are correct, but in the errors 2 and 3 are flipped.

[HDembinski]

It is not a simple transpose, of course, the displacement gets more bizarre if h is not a square matrix.

[jpivarski]

I'm working on the histogram interface right now (PR #196), so I wasn't sure that this would still be true. Here's my reproducer (using a non-square histogram—unfortunately, all the examples in scikit-hep-testdata are square):

>>> import ROOT
>>> h = ROOT.TH2D("h", "", 5, -10, 10, 7, -28, 28)
>>> h.SetBinContent(1, 1, 1)
>>> h.SetBinContent(1, 2, 2)
>>> h.SetBinContent(2, 1, 3)
>>> h.SetBinContent(2, 2, 4)
>>> h.SetBinError(1, 1, 1)
>>> h.SetBinError(1, 2, 2)
>>> h.SetBinError(2, 1, 3)
>>> h.SetBinError(2, 2, 4)
>>> f = ROOT.TFile.Open("issue-198.root", "recreate")
>>> h.Write()
302
>>> f.Close()

Reading it back in Uproot (PR #196, flow=False is now the default):

>>> import uproot4
>>> h = uproot4.open("issue-198.root:h")
>>> h.values()
array([[1., 2., 0., 0., 0., 0., 0.],
       [3., 4., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.]])
>>> h.errors()
array([[1., 2., 0., 0., 0., 0., 0.],
       [3., 4., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.]])
>>> hb = h.to_boost()
>>> hb.view().value
array([[1., 2., 0., 0., 0., 0., 0.],
       [3., 4., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.]])
>>> hb.view().variance**0.5
array([[0., 0., 0., 0., 0., 2., 4.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.]])

Yup. There it is. This is probably related to #93. After fixing this, I'll check across all the histogram types to be sure the transposition is applied generally.

[jpivarski]

This commit, afe8940, results in:

>>> import uproot4
>>> h = uproot4.open("issue-198.root:h")
>>> h.values()
array([[1., 2., 0., 0., 0., 0., 0.],
       [3., 4., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.]])
>>> h.errors()
array([[1., 2., 0., 0., 0., 0., 0.],
       [3., 4., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.]])
>>> hb = h.to_boost()
>>> hb.view().value
array([[1., 2., 0., 0., 0., 0., 0.],
       [3., 4., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.]])
>>> hb.view().variance**0.5
array([[1., 2., 0., 0., 0., 0., 0.],
       [3., 4., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0.]])

[jpivarski]

I also made sure that all output arrays are in native endianness. (I think boost-histogram required that, but it's good for values and errors to return the same thing.)

[HDembinski]

Thank you for the immediate fix! Yes, we require native endianness, but so does ROOT itself, no? The memory buffer in a TH* is always in native endianness.

Also good to hear that flow=False is the default.

[HDembinski]

Should I try out the develop branch to see whether this fixed my problem before we close this?

[jpivarski]

This and #167 are both linked to #196, so they'll be closed when that gets merged. I'm attempting to get the histogram interface finalized—it would be a generalization of what you and @henryiii are converging to (i.e. additional methods and arguments are allowed).

You can try out the development branch, but be aware that the interface has changed in it.