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

Fix histograms for Gaudi v39 and add a configurable histogram #239

Merged
merged 6 commits into from
Sep 25, 2024

Conversation

jmcarcell
Copy link
Member

@jmcarcell jmcarcell commented Sep 20, 2024

BEGINRELEASENOTES

  • Fix histograms for Gaudi v39. After https://gitlab.cern.ch/gaudi/Gaudi/-/merge_requests/1586 the "old" histograms are renamed to StaticRootHistogram from RootHistogram
  • Add a configurable histogram: RootHistogram, whose bins, title and other properties can be set from python as shown in the test file

ENDRELEASENOTES

@m-fila
Copy link
Contributor

m-fila commented Sep 20, 2024

The upcoming version of Gaudi is v39

@jmcarcell
Copy link
Member Author

True, fixed

@jmcarcell jmcarcell changed the title Fix histograms for Gaudi v40 and add a configurable histogram Fix histograms for Gaudi v39 and add a configurable histogram Sep 22, 2024
jmcarcell added a commit to key4hep/k4Reco that referenced this pull request Sep 22, 2024
RootHistogram has been renamed to StaticHistogram, see
key4hep/k4FWCore#239
public:
#if GAUDI_MAJOR_VERSION >= 39
// This is a histogram with title, name and bins that can be set from python
void registerCallBack(Gaudi::StateMachine::Transition, std::function<void()>) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is necessary for any algorithm that has histograms? In that case we need to document it.

Copy link
Member Author

@jmcarcell jmcarcell Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary for histograms with configurable properties, I added a comment about that (so the ones that are not Static...).

@tmadlener
Copy link
Contributor

Works for me against v39r0 locally. Anything still missing for this?

@jmcarcell
Copy link
Member Author

Nothing missing, there is one histogram in k4RecTracker that you found and I think some in one PR in k4GaudiPandora and that should be all left. I'll enable auto-merge.

@jmcarcell jmcarcell enabled auto-merge (squash) September 25, 2024 16:38
@jmcarcell jmcarcell merged commit 4a6bcc1 into main Sep 25, 2024
4 of 9 checks passed
@jmcarcell jmcarcell deleted the fix-histogram branch September 25, 2024 16:45
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