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

[14_1_X Phase2 SIM] Fixed Birks saturation constants for HGCal scintillators #44611

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

civanch
Copy link
Contributor

@civanch civanch commented Apr 4, 2024

PR description:

The value of the main Birks saturation coefficient is taken from classical publications (NIM 80 (1970) 239-244), in which a biased value of the main term was proposed. It was fixed before Run2 for Hcal and now is fixed for HGCScintSD.

Regression for Phase-2 WFs is not expected.

PR validation:

private

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for: likely NO

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44611/39800

  • This PR adds an extra 32KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2024

A new Pull Request was created by @civanch for master.

It involves the following packages:

  • SimG4CMS/Calo (simulation)
  • SimG4Core/Application (simulation)

@mdhildreth, @civanch, @cmsbuild can you please review it and eventually sign? Thanks.
@fabiocos, @bsunanda, @makortel, @rovere, @ReyerBand, @slomeo, @felicepantaleo, @thomreis, @wang0jin this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

@civanch
Copy link
Contributor Author

civanch commented Apr 4, 2024

please test

@civanch
Copy link
Contributor Author

civanch commented Apr 4, 2024

assign upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2024

New categories assigned: upgrade

@srimanob,@subirsarkar you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2024

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-194b19/38598/summary.html
COMMIT: 5700322
CMSSW: CMSSW_14_1_X_2024-04-04-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/44611/38598/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

@civanch
Copy link
Contributor Author

civanch commented Apr 4, 2024

+1

results should be affected, because scintillation hits are slightly different

@srimanob
Copy link
Contributor

srimanob commented Apr 5, 2024

Hi @civanch

Thanks for the fix. When you said that result should be effected, do you think the fix should be backport for 14_0 production? Do we have validation outside this PR? FYI @cms-sw/hgcal-dpg-l2 @SohamBhattacharya @rovere

@civanch
Copy link
Contributor Author

civanch commented Apr 5, 2024

@srimanob , @cms-sw/hgcal-dpg-l2 @SohamBhattacharya @rovere, this fix provides a small effect on HGCal hits/digi and I would expect change <~1% of jet energy. In HCal similar fix changed visible energy for hadrons for ~1-2% and improved MC/data agreement on % level. In HCal scintillator is the main sensitive detector, in HGCal it is not the case.

So, we are speaking on a minor thing. In that case, I would prefer to have theoretically more correct variant, also it is safe to backport.

@srimanob
Copy link
Contributor

srimanob commented Apr 5, 2024

Hi @civanch
Thanks for explanation. If it is very minor, I would propose to have it in master only. So we can validate it when the new pre-release comes. So, we keep production as is for the moment.

@srimanob
Copy link
Contributor

srimanob commented Apr 5, 2024

+Upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2024

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

@civanch
Copy link
Contributor Author

civanch commented Apr 5, 2024

@srimanob , OK, agreed.

@pfs
Copy link
Contributor

pfs commented Apr 5, 2024

From the RecHit spectrum in the scintillator region one can see that indeed it has a very small impact
(e.g. here)
Thanks @civanch for this correction

@antoniovilela
Copy link
Contributor

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants