Dead Code in Simulator failing to add parameters to the RunHeader
Describe the bug
The addition of parameters in Simulator::onFileClose do not get persisted into the on-disk RunHeader since we only modify a local copy of the RunHeader instead of modifying the RunHeader in-place within the output file.
https://github.com/LDMX-Software/ldmx-sw/blob/69f01ee406fa60518c9d0cf1e2ba551040aace8c/SimCore/src/SimCore/Simulator.cxx#L170-L173
This should be auto& instead of auto. :face_exhaling:
Introduced by me in https://github.com/LDMX-Software/SimCore/pull/66 included in v1.0.0 of SimCore, meaning ldmx-sw <= v3.1.11 does not have this bug and >= v3.1.12 does.
To Reproduce Steps to reproduce the behavior:
- Run any simulation
- Open RunHeader written to output file
- See that neither
"Event Count"nor"Events Began"are included in theintParameters_
Desired behavior Since both the total event count and the number of events began are tracked elsewhere, I suggest we just delete these lines to avoid confusion in the future.
Additional context
I found this while trying to do an EoT calculation on samples produced with ldmx-sw v3.3.2. They don't have the numTries_ member of the RunHeader, but I was expecting them to have these other intParameters_.
are tracked elsewhere
for completeness maybe you could point to where?
The "Event Count" is equivalent to the number of events in the output file[^1] which can be retrieved from the LDMX_Events TTree by getting the number of entries.
The "Events Began" is the numTries_ member of the RunHeader in the LDMX_Run TTree in the output files.
[^1]: Technically, "Event Count" that was reported here would include events that passed the simulation filters but then would later be dropped by other processors (e.g. if the event failed the trigger and trigger skimming was enabled). Maybe having a passing-simulation event count separate from a passing-all-processors event count is helpful? I think it lends itself to more confusion and folks who want to study the trigger efficiency (for example) should not be trigger skimming anyways.
OK I agree it's better to remove this now instead of re-introducing it