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

Ensure electron mass does not go to 0 after FastSim bremsstrahlung #42236

Merged

Conversation

sbein
Copy link
Contributor

@sbein sbein commented Jul 12, 2023

Tracked down a cause of the seg fault reported here #24051, found that FastSim bremsstrahlung emissions were being treated like decays (e->e+gamma), where the 4-vector of the photon is subtracted from the electron, but this causes the electron mass to go to zero approximately once in 10k events. Fixed code to preserve electron mass to prevent it from going to 0. CMS is not really sensitive to the electron mass, so the change to physics should be negligible.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 12, 2023

A new Pull Request was created by @sbein (Sam Bein) for CMSSW_10_6_X.

It involves the following packages:

  • FastSimulation/SimplifiedGeometryPropagator (fastsim)

@cmsbuild, @ssekmen, @civanch, @mdhildreth, @sbein can you please review it and eventually sign? Thanks.
@matt-komm this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

@sbein please submit your fix in the master release first, and then percolate it through the still active releases till this 10_6

@sbein sbein changed the title ensure electron mass does not go to 0 after bremsstrahlung Ensure electron mass does not go to 0 after FastSim bremsstrahlung Jul 12, 2023
@sbein
Copy link
Contributor Author

sbein commented Jul 21, 2023

Given this was merged, can we merge this backport?
#42250 (comment)

@sbein
Copy link
Contributor Author

sbein commented Jul 25, 2023

please test

@sbein
Copy link
Contributor Author

sbein commented Jul 25, 2023

percolat

I'm just pinging because this release is high priority for the UL campaigns.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-63ce54/33870/summary.html
COMMIT: a9022f2
CMSSW: CMSSW_10_6_X_2023-07-23-0000/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/42236/33870/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 8 lines to the logs
  • Reco comparison results: 3295 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 3215686
  • DQMHistoTests: Total failures: 5710
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3209642
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 144 log files, 103 edm output root files, 35 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

backport of #42250

@perrotta
Copy link
Contributor

I'm just pinging because this release is high priority for the UL campaigns.
@sbein I think you are pinging yourself, because you are one of those supposed to sign for FastSim

In any case, 10_6_X is a closed release in which FastSim samples were already produced: in that case this fix which changes the FastSim output need to be protected with a modifier

By the way, don't you think that also the other intermediate releases in which fastsim is run can profit of this fix? It could be better to backport now rather than realizing only once the productions are started that you actually need it...

@sbein
Copy link
Contributor Author

sbein commented Aug 1, 2023

@perrotta this is the preferred fix which only affects 1/10k events for a very soft electron - the change to physics is virtually non-existent and only affects the de/dx collections, which are not saved in the standard collections. Furthermore, it passes all tests. SUS needs this to achieve approval of their requests, so I think it would be good to merge as is. Yes, this will be percolated to other production releases, but this is the release corresponding to the currently stuck requests.

@srimanob
Copy link
Contributor

srimanob commented Aug 1, 2023

In 12_4 (2022 FastSim) and 13_0 (2023 FastSim), I assume we don't need modifier right? As FastSim has not started yet.

@srimanob
Copy link
Contributor

srimanob commented Aug 2, 2023

Hi @sbein @perrotta
To conclude from what I understand,

Thx.

@perrotta
Copy link
Contributor

perrotta commented Aug 2, 2023

  • I did not make backport to 12_5, 12_6, 13_1 as we should close them soon. Please let me know if I should just do. It is very simple anyways.

Thank you @srimanob !
Yes, I would skip 12_5 and 12_6.
About 13_1, it is being used for Phase2 MC: I don't think there is any possible usage of FastSim for Phase 2, but if there are doubts I would make the additional little effort of backporting also in 13_1 (no modifiers needed)

@srimanob
Copy link
Contributor

srimanob commented Aug 2, 2023

13_1 backport: #42453

@sbein
Copy link
Contributor Author

sbein commented Aug 7, 2023

@perrotta
I did a private validation and looked at every NANO variable before and after:
https://www.desy.de/~beinsam/FastSim/Nano/7Aug2023/C1C1_Fast_C124Xmore/index.html
As expected, there is not a single statistically significant deviation that I saw looking at every plot. This fix just prevents a seg fault and I don't think it warrants needing a modifier - there's no reason to preserve the ability to recreate the seg fault in future releases, in my opinion. But you have the last say if you still object, I have finished my studies I had in mind.

@sbein
Copy link
Contributor Author

sbein commented Aug 9, 2023

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2023

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_6_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_13_3_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 2c7c8d7 into cms-sw:CMSSW_10_6_X Aug 17, 2023
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.

5 participants