Weird memory bug with waveform simulator

[S81D]

Describe your changes

The PMTWaveformSim tool works as intended in the current, main repo. One feature of the simulator is the random time smearing that is applied to the MCHits before waveform template construction is performed. The time smearing is based on the laser data measurements, and is populated in the store by the LoadGeometry tool. The waveform simulator will grab the timing uncertainty by using the PMT ID to access the map.

Everything works fine, except when this tool is used in combination with the LoadReweightGENIEEvent tool. For some reason the tool can no longer grab the timing uncertainties (using the channel key) from the store:

ToolAnalysis/UserTools/PMTWaveformSim/PMTWaveformSim.cpp

Line 543 in 557f8b8

auto it = ChannelKeyToTimingSigmaMap->find(pmtid);

hit charge =  1.222399 p.e., hit time =  65.417686 for PMTID 462
[2]: No timing sigma for PMT 462, using default
PMTWaveformSim:
    hit charge =  0.680517 p.e., hit time =  47.844757 for PMTID 463
[2]: No timing sigma for PMT 463, using default
PMTWaveformSim:

The map is blank and the value returned is nonsense (e-333) and the simulator instead returns the "default" value of 1ns. The performance of the code is not stable (it works, then re-making the exact same code causes it to not work). I assume its some weird memory issue when using auto it and that the initialization is done with a pointer, but I can't find any reliable solutions. I'm making it seem more supernatural than it is but I have tried many different modifications and none reliably solve the problem.

The proposed solution is to add the timing uncertainties ordinarily loaded into the store by the LoadGeometry tool to the Lognorm csv file already utilized for template construction. This sidesteps the problem and the tool is able to reliably access the timing uncertainties.

I also threw in a fatal error if the timing uncertainty cannot be grabbed - without it, it could lead to bias in vertex reconstruction if someone thinks the tool is applying time smearing (if time smearing is enabled in the config) and doesn't actually check the print out.

This fix is stable with the reweight tool and works as intended. Tested over many files.

Checklist before submitting your PR

• This PR implements a single change (one new/modified Tool, or a set of changes to implement one new/modified feature)
• This PR alters the minimum number of files to affect this change
• [N/A] If this PR includes a new Tool, a README and minimal demonstration ToolChain is provided
• [N/A] If a new Tool/ToolChain requires model or configuration files, their paths are not hard-coded, and means of generating those files is described in the readme, with examples provided on /pnfs/annie/persistent
• For every new usage, there is a reason the data must be on the heap
• For every new there is a delete, unless I explicitly know why (e.g. ROOT or a BoostStore takes ownership)

^ for the last two I am setting the random seed with fRandom = new TRandom3();.

Additional Material

Attach any validation or demonstration files here. You may also link to relavant docdb articles.

[marc1uk]

well.... this works, i guess.

Some very minor things that seem like changes that aren't necessarily for the better:

• not sure why fRandom was moved to the heap... i'm guessing this was just a random experiment that didn't help and never got reverted
• Same with the functionality of the TimeSmearing method being split up so it doesn't actually return the time jitter, but sets a member with the uncertainty, which is then sampled to obtain a smearing value outside the function... it seems like better functional encapsulation to have it just return the smearing as it used to.
  Perhaps this change was made to accommodate the success return value from TimeSmearing, but it could have just accepted a smearing value by refererence...
• The new return boolean from TimeSmearing isn't actually checked, so although the logmessage prints "exiting...", i don't actually see anything that makes it exit. Maybe there's a missing call here to m_data->vars.Set("StopLoop",true); or something?

At least only the last one seems like a necessary fix.

However.... it is a bit of a worrisome workaround. That the current code stops working suggests there may be memory corruption, and burying our heads in the sand (and our lead on that corruption) is unlikely to be the best approach.
Recommended procedure would be:

1. check the map to see if the channel is known
2. if it is not known, print the contents of the map and terminate. Is the map empty now? has it been corrupted?
3. print the map when it is first loaded, and at the start of each Execute. Was it valid on first load, but got wiped or corrupted at some later time?
4. print the map in various Tools, and see after which Tool / bit of code it changes. In fact it seems we already believe it's LoadReweightGENIEEvent Tool, so stick printouts at the start and end of that Tool's Execute and see if it changes.
   ...
   etc.

While I appreciate that this could take some time, if there is memory corruption happening, hiding our only lead to it is liable to come back and bite you later, requiring more time investment to get valid results...