MatrixTransferStatic only initalizes matrix once #1566
Conversation
…red MCparticle collections
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Okay, so I wasn't expecting a major change right now - this is not what was discussed in the Mattermost.
Since this code is used for 50% of the FF detectors, I really do not want major changes handled this way. We need to have a discussion about what changes will be made, and why before this is coded up and merged. It makes it very difficult for me if I have to read through new code blocks without knowing the purpose of the changes, and how they might affect future updates.
I wrote this code the way I did to handle the various MC files we deal with (not all MC files handle beam particles the same way, or even properly), and to handle particle gun input (hence the mcparts->size() == 1 conditional). We have to have flexibility for these different options - this flexibility exists for the tested items, and has been verified to work.
I am unfamiliar with the BeamProtons and ScatteredProtons - this seems to only be something which will work if you have a proton beam - what happens when you have a spectator from e + d or e + He3? What about a coherent final state like He4?
Using MCParticles allows us to easily adjust the algorithm to add extra conditions, and simply add the PDG code for the check - using custom classes for a beam particle seems less flexible, and adds more complication to tracking down a simple problem.
|
Additionally, the reason I did not simply use the first particle in the list is because of the beam effects in MCParticles. If I just took the first particle, there's a chance that single particle is outside of the spec for the nominal beam we expect - taking the average over all of the beam protons smoothed out any outliers and ensured that we never threw away an entire set of events. That for loop over the particles does not take any time at all - the RP reconstructions is only ever eventually processing and storing 1, or at most 2 tracks. |
|
I wasn't proposing to implement this without a discussion and would be happy to remove aspects related to separating the proton from the MCParticles. Otherwise only very little has been changed and it was just meant to be a refactoring rather than a major update. Longer term, more algorithms might depend on the identification of specific MCParticles so having them defined in a central location makes sense so it will actually be easier to limit any future discrepancies and can be expanded to handle other cases. |
|
That's fine - I think for now just fixing the issue that was mentioned on the Mattermost (making the algorithm quieter) is totally fine. I think changing the extraction of the beam particle should be discussed at a meeting - perhaps at a combined FF/FB meeting, or maybe even in a reconstruction working group meeting, since it will affect many people. |
|
Should the nominal momentum be set to the value that has been identified by the filter rather than remain as the value calculated from the MCParticles? Maybe that should be a separate bugfix. |
|
Yes, that's a good point. The presently-stored value is that average number used to determine which matrix is needed, which is not the correct number to reconstruct the full momentum with that particular matrix. |
|
Capybara summary for PR 1566 |
Briefly, what does this PR introduce?
From comments https://chat.epic-eic.org/main/pl/qo3sdqkdgibijpetrhxocoio5e this should result in the initialization of the transfer matrix only being carried out once so the logger won't flag up errors every event.
A couple of slight functional changes which will need checking and possibly reverting:
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
No
Does this PR change default behavior?