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

Request for support of writing hist-derived profiles #908

Closed
raymondEhlers opened this issue Jun 21, 2023 · 4 comments · Fixed by #1000
Closed

Request for support of writing hist-derived profiles #908

raymondEhlers opened this issue Jun 21, 2023 · 4 comments · Fixed by #1000
Assignees
Labels
feature New feature or request

Comments

@raymondEhlers
Copy link
Contributor

I'd like to request writing support for profile hists that are coming from hist/boost-histogram. The use case I ran into was merging some root files containing a variety of histograms. My procedure was roughly:

  1. Read file
  2. Retrieve hists, and convert using to_hist to take advantage of the __add__ functionality already implemented
  3. Add the hists
  4. Write the merged hists to file

The issue is in step 4. This works fine for standard histograms, and from the docs I understand that it should work for TProfile objects that aren't converted to hist (eg. just read by uproot, and still contained in a Model), but it doesn't work after I've done the conversion. Looking into the exception that's raised

https://github.com/scikit-hep/uproot5/blame/a9f4374607b7434ef89ebec82c784d34bfc31413/src/uproot/writing/identify.py#L248-L255

the NotImplementedError obviously suggests that profiles intentionally aren't supported at the moment, but the ValueError suggests that perhaps they were intended to be? In any case, adding support for writing profiles would be much appreciated, and would make merging hists a bit easier (this would be helpful because hadd isn't always available). Thanks!

@raymondEhlers raymondEhlers added the feature New feature or request label Jun 21, 2023
@jpivarski
Copy link
Member

I've had this in my email inbox for a week in order to say something intelligent about it, but all I can say is, yes: that would be a very good feature to have. Hopefully, we'll be able to get more effort on Uproot because there are a few important requests like this one.

I'll just note that this is a high-priority item.

@raymondEhlers
Copy link
Contributor Author

Thanks for your response! I know that I'm making a request for a new feature without offering to actually help on the implementation, so I have no expectations about the timeline. (From what I can tell, it needs some expertise on PlottableHistogram that I don't have, but if you think it's something that could be completed in a couple of hours for someone new to the uproot internals, I'm willing to give it a shot).

As always, thanks for all of your efforts!

@ioanaif ioanaif self-assigned this Oct 5, 2023
@ioanaif ioanaif linked a pull request Oct 19, 2023 that will close this issue
@lobis lobis changed the title Reuqest for support of writing hist-derived profiles Request for support of writing hist-derived profiles Oct 19, 2023
@jpivarski
Copy link
Member

PR #1000 ensures that TProfiles can be read from ROOT and written to ROOT with 100% fidelity. It doesn't show that they can be taken from or to hist/boost-histogram, but that's a separate issue. When this is closed by PR #1000 getting merged, you may still want to check that.

The thing I'm uncertain about is whether hist/boost-histogram's WeightedMean histograms have enough per-bin data to represent a ROOT (HBOOK, SUMX, ...)-style "profile plot," which is more involved than simply tracking the weighted mean of each bin.

@raymondEhlers
Copy link
Contributor Author

Thanks for implementing this! I ran some tests as a brief follow up and writing hist objects works as requested, thanks!

For my overall goal of merging and saving profiles, I think something is still going wrong around the to/from hist conversion (as you suspected). eg. If I add two WeightedMean() profiles created via hist, I get the correct answer, but if I read a profile via uproot, convert to_hist and then add them, I get the wrong answer. In any case, the new writing support is good progress, and I'll have to dig into this further in another issue some other time

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

Successfully merging a pull request may close this issue.

3 participants