Skip to content
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

add histogram LGDO #98

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

add histogram LGDO #98

wants to merge 6 commits into from

Conversation

ManuelHu
Copy link

@ManuelHu ManuelHu commented Jul 10, 2024

Some open questions:

  • this currently has a hard import on the hist package, which could also be a strictly optional dependency. see also Handling package dependencies required by LGDO.view_as() #48
  • I added an error message when trying to write a histogram with any wo_mode that might append. I am unsure if this is the nice way to do it...
  • For Histogram, not being able to add/remove fields is a hard requirement - otherwise it cannot be read back as a histogram. But overriding all the custom functionality of Struct is not nice. Maybe, having a base class for Struct, but without a clear way for the user to add/remove fields would make sense?
  • Should a HistogramAxis accept attrs?
  • Should lgdo.types.HistogramAxis be moved to lgdo.types.Histogram.Axis (i.e. as nested type, as the axis type does not make sense as a standalone LGDO?)

fixes #15

@ManuelHu ManuelHu force-pushed the histogram branch 2 times, most recently from e5ee7fd to 58f5eae Compare July 10, 2024 08:49
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 97.12919% with 6 lines in your changes missing coverage. Please review.

Project coverage is 76.05%. Comparing base (5ac36c8) to head (a2f585e).
Report is 1 commits behind head on main.

Files Patch % Lines
src/lgdo/lh5/_serializers/read/composite.py 80.95% 4 Missing ⚠️
src/lgdo/types/histogram.py 98.89% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
+ Coverage   74.45%   76.05%   +1.59%     
==========================================
  Files          45       46       +1     
  Lines        2815     3024     +209     
==========================================
+ Hits         2096     2300     +204     
- Misses        719      724       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ManuelHu ManuelHu marked this pull request as ready for review July 10, 2024 14:33
@gipert gipert added enhancement New feature or request types LGDO types lh5 HDF5 I/O labels Jul 11, 2024
@gipert
Copy link
Member

gipert commented Jul 11, 2024

Looks great Manuel! Some comments:

  • we should also support variable-binning histograms
  • I would ignore the "problem" with mutable Structs for now
  • I think going for lgdo.types.Histogram.Axis is a good idea
  • could you document a bit better if data is copied or actually viewed?

@oschulz:

src/lgdo/types/histogram.py Outdated Show resolved Hide resolved
@ManuelHu
Copy link
Author

@oschulz
Copy link

oschulz commented Jul 11, 2024

https://legend-exp.github.io/legend-data-format-specs/dev/hdf5/#Histograms does not mention histograms with variable binning, but should be straightforward to have binedges as an array<1>{real}.

Yes, we should absolutely allow that.

could you provide an LH5 file with a Histogram object serialized with Julia?

I'll add a test file to legend-testdata, give me a few days ...

@ManuelHu
Copy link
Author

ManuelHu commented Jul 11, 2024

Variable binning works now (with array<1>{real} as binedges). I can also prepare a PR to the data specification in the next days.
There is also some logic that converts an edge array to a range object, if this is possible (i.e. it has a uniform binning). I Am not sure if we want to keep that behavior...

but please do not merge yet, I still have one change in mind. I still want to change one possible parameter type of the binning, as now it is not so nice. -> implemented

@oschulz
Copy link

oschulz commented Jul 12, 2024

I can also prepare a PR to the data specification in the next days.

Can you also add a test file to the legend-testdata repo via a PR there, so we can test on the Julia side before we merge here and on legend-testdata?

@ManuelHu ManuelHu force-pushed the histogram branch 2 times, most recently from 61cf866 to e77af46 Compare July 13, 2024 10:18
ManuelHu added a commit to ManuelHu/legend-testdata that referenced this pull request Jul 13, 2024
@ManuelHu
Copy link
Author

@oschulz see legend-exp/legend-testdata#18
I now uploaded a test file with two 2D histograms, one with range axes, and one with variable binned axes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lh5 HDF5 I/O types LGDO types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement histogram LH5 specification
3 participants