-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat: support categorical axes on boost histograms #764
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Angus Hollands <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…ld be mostly equal)
for more information, see https://pre-commit.ci
At this point serialization appears to work okay but the axis is not being drawn in the histogram (drawn using root). This is probably due to some badly set option. I will compare the uproot generated histogram with one generated with root to see the differences.
|
Co-authored-by: Jim Pivarski <[email protected]> Co-authored-by: Angus Hollands <[email protected]>
for more information, see https://pre-commit.ci
# Conflicts: # src/uproot/behaviors/TH1.py # src/uproot/behaviors/TH2.py # src/uproot/behaviors/TH3.py # tests/test_0422-hist-integration.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It all looks good and I approve. Thank you very much for the effort you put into this!
One thing now looks odd to me: you've removed the three instances of
def to_boost(self, metadata=None, axis_metadata=None):
return super().to_boost(metadata=metadata, axis_metadata=axis_metadata)
which bypass the no_inherit
mechanism, which prevents normal inheritance because of the ROOT histogram inheritance tree. So how is TH1D getting a to_boost
method?
I can see that it is because these tests pass:
(I was about to add them, but they were already there.)
Nevermind—I think I see it: Tthe Python inheritance is graph TB
A((Histogram))-->B((TH1))
A-->C((TH2))
A-->D((TH3))
A-->H((Profile))
H-->E((TProfile))
H-->F((TProfile2D))
H-->G((TProfile3D))
and the C++ inheritance is When we deserialize a So this PR makes complete sense to me. I'll squash-and-merge it. Thanks again! |
I will merge (squash!) once the tests pass. |
Closes #672.
The logic for setting the
fLabels
on the axis is pretty straightforward, however to achieve this we need to serializeTHashList
which is not currently implemented.While working on this I found out a problem when serializing
TList
(whichTHashList
inherits directly from), which is pointed out in #762 and is fixed by #763.Another issue related to weighted histograms was also found and fixed at #774.
Summary of changes:
THashList
serialization andto_HashList
factory method.THashList
writing. Compare serialization (bytes) toTList
as they should be the same except for class name.fName
) of all axes is nowxaxis
,yaxis
orzaxis
regardless of user definition, as this field should not be modified by the user since it causes problems on histogram drawing. (The text of the axis isfTitle
). Tests have been updated accordingly.