Skip to content
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

Merged
merged 5 commits into from
Jan 22, 2024
Merged

Conversation

baltzell
Copy link
Collaborator

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?

@baltzell
Copy link
Collaborator Author

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.

@cbravo135 cbravo135 linked an issue Apr 19, 2023 that may be closed by this pull request
@cbravo135
Copy link
Collaborator

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.

@baltzell
Copy link
Collaborator Author

I don't think we want to be looking at tracking for this.

@normangraf
Copy link
Contributor

I don't think we want to be looking at tracking for this.

I concur.

@normangraf
Copy link
Contributor

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.

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.

@cbravo135
Copy link
Collaborator

cbravo135 commented Apr 25, 2023

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.

@baltzell
Copy link
Collaborator Author

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?

@baltzell
Copy link
Collaborator Author

baltzell commented May 3, 2023

Things are looking better now, see screenshot. Time for some more simulations to look at simple Mollers plus the pair/positron trigger turn on curve.
Screen Shot 2023-05-03 at 10 54 26 AM

@bloodyyugo
Copy link
Contributor

Looks pretty great...thanks for looking at this.

@tongtongcao
Copy link
Contributor

The update for DigitizationWithPulserDataMergingReadoutDriver is similar to the update for DigitizationReadoutDriver.
For pulser data overlay, there are two difference cases:

  1. if a ADC sample is directly from random data, or merged with random data, we do not need to do anything, such as smearing hit energy, adding noise, and adding pedestal.
  2. if a ADC sample is from MC signal, no merging with random data, we should do the same thing as we did for MC signal + MC beam as background.

@@ -48,7 +48,7 @@ public EcalDigitizationReadoutDriver() {
setTriggerPathTruthRelationsCollectionName("TriggerPathTruthRelations");
setReadoutHitCollectionName("EcalReadoutHits");

setPhotoelectronsPerMeV(32.8);
setPhotoelectronsPerMeV(30.8);
Copy link
Member

@JeremyMcCormick JeremyMcCormick May 18, 2023

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.

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

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.

@tongtongcao
Copy link
Contributor

Analysis group will test effects of Ecal smearing updates. Let's wait for their feedbacks.

Copy link
Collaborator

@cbravo135 cbravo135 left a 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

@cbravo135 cbravo135 marked this pull request as ready for review January 22, 2024 17:40
@cbravo135 cbravo135 merged commit 81ebe3e into master Jan 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tune/fix ECAL simulated energy resolution to match real data
6 participants