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

[hegner]

BEGINRELEASENOTES

• Add GeneratorEventParameters and GeneratorPdfInfo datatypes to store generator related data in a structured and well defined way.
• Add a GeneratorToolInfo struct and related utility functionality to store some high level metadata on the generator into Frame parameters.

ENDRELEASENOTES

Pull request to collect all the changes required for #309

[tmadlener]

I think overall this looks like a reasonable structure (at least to me). But @dirkzerwas should probably have a look to make sure we don't miss anything obvious yet.

Can you also add tests to read back the things that you currently write. At least the ToolInfo parts have enough additional logic wrt podio to warrant a test, I think.

[dirkzerwas]

Hi @hegner and @tmadlener,

looks very nice, a couple of thoughts:

• 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)
• 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)
• 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?

Cheers,
Dirk

[hegner]

• 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]

@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]

@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" :)

• 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?
• I would prefer to keep the crossSections where you have put them and not extract them, the structure is clearer

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

[hegner]

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]

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]

@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?

some more minor changes

[tmadlener]

In case any of these parameters is not available this implementation will throw a std::bad_optional_access, which might be a bit cryptic, but would at least be in line with "fail early in case of problems".

If we wanted a slightly more relaxed approach without exceptions escaping, we could do:

Suggested change

	const auto names = frame.getParameter<std::vector<std::string>>(generatorToolName).value();
	const auto versions = frame.getParameter<std::vector<std::string>>(generatorToolVersion).value();
	const auto descriptions = frame.getParameter<std::vector<std::string>>(generatorToolDescription).value();
	const auto names = frame.getParameter<std::vector<std::string>>(generatorToolName).value_or({});
	const auto versions = frame.getParameter<std::vector<std::string>>(generatorToolVersion).value_or({});
	const auto descriptions = frame.getParameter<std::vector<std::string>>(generatorToolDescription).value_or({});

In that case the worst that can happen is an empty vector, which probably also shows that something went wrong.

I am in favor of the current implementation as that makes errors harder to ignore and easier to diagnose (even if the std::bad_optional_access might be a bit cryptic).

[tmadlener]

After some more discussion with @dirkzerwas about how this will be used, I think we can do the "error" handling internally, and simply return an empty vector instead of letting the exception propagate. The main reasons are

• vector::empty still let's users check whether information was available
• Information on the tools rather falls into a "nice to have" category, rather than crucially important information for event processing.

[hegner]

value_or should do then the right thing then

[tmadlener]

A few more things triggered by the clashing variable and class names.

[tmadlener]

These are now clashing with the generated classes. Maybe all of these should just be prefixed with Gen instead of Generator?

[hegner]

I think I will do that indeed. The label helped as well before ;-)

[andresailer]

Or we add a namespace for the constants.
And we copy the existing constants into the namespace and deprecate the namespaceless ones (we can even align spelling when we do that without breaking anything)

[hegner]

that would be a major change and a bit cumbersome if dealing e.g. with the matrices end the corresponding enums. Do you really want to do that?

[m-fila]

IIRC the enums were supposed to stay where they are, since they aren't constants

[tmadlener]

I would like a edm4hep::label namespace for things that are a label. the enums for covariance matrix access can / should stay in the edm4hep namespace.

[tmadlener]

See #315 for a proposal with a new labels namespace.

[tmadlener]

The classes we define in the yaml file are all prefixed Generator, but this one is Gen. For consistency reasons, I think they should both be the same. The choice is also related with how we want to name the constants.

[tmadlener]

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

[hegner]

Perfect. Thank you

[tmadlener]

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

[dirkzerwas]

@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(....)