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 file with LGDO-serialized histograms #18

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

ManuelHu
Copy link
Contributor

@ManuelHu ManuelHu commented Jul 13, 2024

Created with legend-pydataobj w/ legend-exp/legend-pydataobj#98 applied

the lh5 file contain two histograms, one with binning defined via a range object, and one with a variable binning (the latter is not mentioned yet in the lh5 spec!)

the data is just a randomly generated 2d gaussian.

@oschulz
Copy link
Contributor

oschulz commented Jul 15, 2024

@apmypb can you test this file in Julia with LegendHDF5IO?

@ManuelHu
Copy link
Contributor Author

I added a third test histogram, now also containing units on the axes.

@oschulz
Copy link
Contributor

oschulz commented Jul 17, 2024

I added a third test histogram, now also containing units on the axes.

Thanks, I'll try to get this tested with Julia soon.

@apmypb
Copy link

apmypb commented Jul 28, 2024

Variable binning works too. Using units however does not work yet, because one can not fit a histogram with units (see JuliaStats/StatsBase.jl#857).
Just to make sure. Is this the histogram in "test_histogram_variable":
image

@oschulz
Copy link
Contributor

oschulz commented Jul 29, 2024

@ManuelHu ?

@ManuelHu
Copy link
Contributor Author

ManuelHu commented Jul 29, 2024

looks good. here are (both histograms) plotted from python:

image

Using units however does not work yet, because one can not fit a histogram with units

Also no python histogram library (that I know of) supports units on axes. So from the python side, this is also a purely informational attribute.
Do I understand it correctly, that you cannot read the histogram with units (which would be very bad), or are just the units missing in the end (which would be the same as with python)?

@apmypb
Copy link

apmypb commented Jul 29, 2024

Exactly like that. The units are just dropped during read in. The actual values of the arrays or ranges are read in correctly and can be displayed accordingly.

@oschulz
Copy link
Contributor

oschulz commented Jul 29, 2024

Then I suggest we merge this - @apmypb can you then add reading these histograms to the LegendHDF5IO.jl tests?

@ManuelHu ManuelHu marked this pull request as ready for review July 29, 2024 17:29
@ManuelHu
Copy link
Contributor Author

I just renamed test_histogram_variable_w_attrs to test_histogram_range_w_attrs (there never was a variable histogram with units in the file). After this, this PR is now ready from my side.

I will also add the tests to my legend-pydataobj pull request.

@ManuelHu
Copy link
Contributor Author

ManuelHu commented Aug 2, 2024

@oschulz can we get this merged?

@oschulz
Copy link
Contributor

oschulz commented Aug 2, 2024

I'll take a final look myself today, then I'll merge it. Sorry for the delay.

@oschulz oschulz merged commit 82ad6c2 into legend-exp:main Aug 5, 2024
@oschulz
Copy link
Contributor

oschulz commented Aug 5, 2024

Thanks again @ManuelHu and sorry for the delay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants