LArSoft

I confirm I have seen a burst of these messages appearing on a single ICARUS event out of 5.

And it's smelling bad, with those energy numbers being so similar to unassigned random double precision variables.

This thing was bothering me already, enough that now I have put some time in investigating it.

Commit larsim:f99b849 by Jason Stock added some optional simulation of argon saturation, and changed the type of energy recorded in the backtracking information from total to visible.
Visible energy includes saturation effects, and it is computed at larg4::ISCalculationSeparate::CalculateIonizationAndScintillation():

• if using the same scintillation yield regardless the type of ionising particle (detinfo::LArProperties::ScintByParticleType() false) and medium saturation simulation, the visible energy is computed from the total energy, including saturation effect
• if using the same scintillation yield for all particles and not simulating the saturation, the visible energy is assigned 0 because "I have not made a correct implimentation of this value for anything but EMSaturation" ("I" also being Jason)
• if using scintillation yield according to particle type (detinfo::LArProperties::ScintByParticleType() false), visible energy is left undefined, hence:
  • at the beginning, that load of nonsensical numbers is stored in backtracking
  • after something (?) assigns a value there, it's not clear to me what happens (I did not investigate)

So in short, Jason should decide what is the right behaviour for that part of the code, and act accordingly.

An amend: the only thing that happens that makes the message stop is the mercy of MessageLogger, which is configured with a limit of number of messages.
The visible energy value (larg4::ISCalculationSeparate::fVisibleEnergyDeposition) stays unassigned all the time.

So, apparently replies to redmine update emails don't go to the ticket (don't know why I thought they did), so I thought I was participating in this conversation, but since I was limited to my phone for most of the day I was doing so through e-mail replies.

Yes. I think the short term solution is to guarantee that the energy written to the BTR is always initialized to zero.

I need to go through and make sure that each case writes a meaningful energy long term, but at least assuring that a zero is written in all incomplete cases makes something that both doesn't slow things down with error logs, and is easy to identify in the final reco so people don't draw incorrect conclusions from incomplete records.

Jason Stock wrote:

So, apparently replies to red mine updates don't go to the ticket (don't know why I thought they did), so I thought I was participating in this conversation.

Yes. I think the short term solution is to guarantee that the energy written to the BTR is always initialized to zero.

I need to go through and make sure that each case writes a meaningful energy long term, but at least assuring that a zero is written in all incomplete cases makes something that both doesn't slow things down with error logs, and is easy to identify in the final reco so people don't draw incorrect conclusions from incomplete records.

Also, I will start on this now.

I just got around to pushing my branch. feature/JStock_Issue22339
20a84bbcaf03f399f126000875efe881290a56fc

This provides a well defined behaviour for all modes in OpFastScintillation.cxx

Thanks Jason. This might make this weeks' larsoft release.

Commend to Jason for using the number of the issue in the commit message, that makes it super-easy to look at it from the ticket here.

Just a note: since 0.0 is smaller than epsilon() (see lardataobj:source:lardataobj/Simulation/OpDetBacktrackerRecord.cxx#L70), error/debug messages will still be emitted.

Also, please remove the comment above the `MF_LOG_ERROR` line. None of the message facility logging routines intrinsically throw exceptions.

To address Gianluca's concern, it will only write zero if none of the methods above offer an energy. That should of course not be the case. I would argue that photons coming from a step with 0 energy deposit is in fact worth an error message.

To address Kyle's point. Yes. The comment is wrong. Will changing the comment increment the data product? If so, then I would argue against doing so unless we do decide we want to demote that error message, but that condition should not happen any more with those changes, and I think the error message is useful.

Thank you both for your detailed and helpful feedback!

Forgot to include the issue number in my last commit.

commit fa0a966bb4d62eb77a4658436ecee1b8f1951b4b (HEAD -> feature/JStock_Issue22339_2, origin/feature/JStock_Issue22339_2)
Merge: 405ddf54 535c4374
Author: Jason Stock <jstock@dunegpvm02.fnal.gov>
Date: Fri Apr 12 12:22:25 2019 -0500

Merge commit '535c4374ecc698af9190ed48b4fdce15ae337471' into feature/JStock_Issue22339_2

This is to resolve the issues Lynn encountered making the merge work.

I should expand a bit. With the initial fix, each individual G4Step could no longer write a 0 to the BackTrackerRecords, but steps with many photons could write 0 in the additional photons. This did not show up in testing because I used the radiological model for my test. The issue simply did not arise in my test case.