key4hep / EDM4hep

Generic event data model for HEP collider experiments
https://cern.ch/edm4hep
Apache License 2.0
25 stars 35 forks source link

Add datatypes for storing generator (meta)data in a structured and defined way #310

Closed hegner closed 3 months ago

hegner commented 4 months ago

BEGINRELEASENOTES

ENDRELEASENOTES

Pull request to collect all the changes required for #309

dirkzerwas commented 4 months ago

Hi @hegner and @tmadlener,

looks very nice, a couple of thoughts:

Cheers, Dirk

hegner commented 4 months ago
  • just wondering if adding ''Event" into GeneratorParameters might make sense to make clear it's the "per event" structure and not the general setting of the generator (like the N PYTHIA switches)

Yes. I think that's a fair point. I will do that rename.

  • I may have missed it: HepMC has a vector of weightnames in the run_info part, suggest to foresee that in "run" (even though this is not filled by all generators consistently at the moment)

For this we do not need a new data type at all. My idea was to just have an example for how to do it in a to-be-written doc page. With possibly a constant defined for the name of the weight names.

  • weights: do you wan to leave the weights in the EventHeader? In principle one could argue that weights are linked with the cross section @apricePhy told me that the use is not the some in all generators. So to be safe, one may want to keep the two together?

I left it in one piece for simplicity. No problem to split it in two.

hegner commented 4 months ago

@dirkzerwas - I have a question for clarification. We have event weights stored in our standard edm4hep::EventHeader and I would not like to change its definition. So we could either leave the cross sections in the GeneratorEventParameters or make it an independent type. What do you think?

dirkzerwas commented 4 months ago

@dirkzerwas - I have a question for clarification. We have event weights stored in our standard edm4hep::EventHeader and I would not like to change its definition. So we could either leave the cross sections in the GeneratorEventParameters or make it an independent type. What do you think?

If downstream algs already access the weights, I understand the difficulty, it was more esthetics than "need" :)

However I would move the signal_vertex (oneToManyRelation) out of the PDF structure if possible?

hegner commented 4 months ago

However I would move the signal_vertex (oneToManyRelation) out of the PDF structure if possible?

@dirkzerwas - no problem. It would then be a standalone collection. I was hoping to keep the number of collections small however. If that's your only comment, I will implement that and finish this feature with writing a simple example.

tmadlener commented 4 months ago

the comment came initially at a conversation with @tmadlener (when discussing weights in general a couple of weeks ago), that by adding "weights" to EventHeader this mixes MC and "in data available" information, correct Thomas?

Yes, exactly. In principle an EventHeader should only only contain things that are available on both simulation and real data. From a "historical point of view" we can only store multiple weights in there since #254, so there is not too much historical baggage, and this has not yet been part of a released version of EDM4hep. We will probably not get rid of the single weight in the EventHeader as easily, as that has been there since the beginning. So we could in principle move the weights vector closer to the cross sections without boo much breakage.

dirkzerwas commented 4 months ago

@hegner I would move the OneToManyRelations: edm4hep::MCParticle signal_vertex // pointing into MCParticle collection to GeneratorEventParameters which already has event_scale etc but maybe I misunderstood your comment?

hegner commented 4 months ago

@dirkzerwas - perfect

dirkzerwas commented 4 months ago

There is one point you might want to talk to HepMC and the MC authors about: have a common approach to beam polarization:

tmadlener commented 3 months ago

Resolved the merge conflicts, added a bit of documentation and renamed GenToolInfo -> GeneratorToolInfo for consistency.

hegner commented 3 months ago

Perfect. Thank you

tmadlener commented 3 months ago

Unless there are complaints by tomorrows meeting, I will merge this shortly before that.

dirkzerwas commented 3 months ago

@tmadlener include/edm4hep/GenToolInfo.h you might need to swiitch to plural for: frame.putParameter(GeneratorToolNames, std::move(names)); frame.putParameter(GeneratorToolVersions, std::move(versions)); frame.putParameter(GeneratorToolDescriptions, std::move(descriptions)); and for the triplet of frame.getParameter(....)