-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address simulated ECAL energy resolution #980
Conversation
What all shall we look at to assess this and #978? Of course FEEs, just because they're easy/free. Two-cluster energy sum for Mollers would get sensitive to lower energies (where the problem and effect of this change should be larger) and still give an easily interpretable resolution number. And I think people are also interested in the trigger turn-on, but I haven't thought about how to assess that, other than just plotting cluster energy distribution around the trigger threshold. |
You can look at the the momentum sum of tracks in V0s with an electron positron pair in opposite volumes. You could also look at the positron momentum distribution turn on comparing data and MC. |
I don't think we want to be looking at tracking for this. |
I concur. |
I don't think the FEE trigger is really sensitive to this resolution, since the energy threshold for the trigger was well below the peak. I would look at the low end of the two-cluster esum plot in trident production since what we really want to probe is the positron turn-on curve. |
We absolutely want to look at the tracks as a metric for this, they are how we noticed there is an issue in the first place. The shape of the threshold in tracks is an independent way to validate the trigger resolution. Using methods of validation like this will be critical to fixing the issue at hand. It is analytically a more robust method of validation, since the track momentum is independent of the information used to generate the trigger, we should not just use something fundamentally derived from the same information as a sole source of validation. |
Yeah, I agree, ultimately we'll need tracks for this (but first FEEs and Mollers would be good without tracking), can you set up some metric for this? |
Looks pretty great...thanks for looking at this. |
…gingReadoutDriver
The update for DigitizationWithPulserDataMergingReadoutDriver is similar to the update for DigitizationReadoutDriver.
|
…It might be a typo mistake.
@@ -48,7 +48,7 @@ public EcalDigitizationReadoutDriver() { | |||
setTriggerPathTruthRelationsCollectionName("TriggerPathTruthRelations"); | |||
setReadoutHitCollectionName("EcalReadoutHits"); | |||
|
|||
setPhotoelectronsPerMeV(32.8); | |||
setPhotoelectronsPerMeV(30.8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constant should be pulled out into a class variable and there should probably be a setter on the Driver for it. Some brief comment in the code on how it was derived might also be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We think that the default value 32.8 is wrong since typo mistake. Let's change the default value as 30.8. Otherwise, we need to reset all readout steering files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We think that the default value 32.8 is wrong since typo mistake. Let's change the default value as 30.8. Otherwise, we need to reset all readout steering files.
Undocumented constants like this are a bad practice. I would suggest that this value be documented someplace within the code. Best way is probably putting the value into a variable with some documentation on it. The value could be a constant if it should never be changed by the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the value will not be changed unless Ecal group re-investigate Ecal noise and crystal light yields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The derivation of this value is documented in https://misportal.jlab.org/mis/physics/hps_notes/viewFile.cfm/2014-002.pdf?documentId=2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are better ways to implement this so software developers that do not understand the physics of detectors can follow. This would be better kept in a resource constant file for ecal, I would guess something like this exists. This way if this is needed to be used in the code elsewhere it will clearly only need to be changed in one place. The way this is written it is unclear.
Analysis group will test effects of Ecal smearing updates. Let's wait for their feedbacks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing @JeremyMcCormick's comment
For now, just moving to uncorrelated noise on each sample, while still applying the photoelectron/statistical resolution separately on the pulse integral. Previously the incorrect treatment of the noise effect rendered it zero. Effect should be largest at lower energies, but no feeling for scale.
Need to get some simulations run to assess. Can anyone help with this?