search

LDMX-Software / ldmx-sw

The Light Dark Matter eXperiment simulation and reconstruction framework.
https://ldmx-software.github.io
GNU General Public License v3.0
20 stars 16 forks source link

Dead Code in Simulator failing to add parameters to the RunHeader #1324

Closed tomeichlersmith closed 1 week ago

tomeichlersmith commented 3 weeks ago

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

tvami commented 3 weeks ago

are tracked elsewhere

for completeness maybe you could point to where?

tomeichlersmith commented 3 weeks ago

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.

tvami commented 3 weeks ago

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