ldmx-sw icon indicating copy to clipboard operation
ldmx-sw copied to clipboard

Dead Code in Simulator failing to add parameters to the RunHeader

Open tomeichlersmith opened this issue 1 year ago • 3 comments

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:

  1. Run any simulation
  2. Open RunHeader written to output file
  3. See that neither "Event Count" nor "Events Began" are included in the intParameters_

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_.

tomeichlersmith avatar May 07 '24 17:05 tomeichlersmith

are tracked elsewhere

for completeness maybe you could point to where?

tvami avatar May 07 '24 19:05 tvami

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.

tomeichlersmith avatar May 07 '24 20:05 tomeichlersmith

OK I agree it's better to remove this now instead of re-introducing it

tvami avatar May 07 '24 20:05 tvami