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

Process modifier for displaced SUSY simulation fix #47197

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

24LopezR
Copy link
Contributor

PR description:

This PR is a follow up of PR #47165 . It adds a process modifier for automatically activating the flag "IsSlepton", needed for production of signal samples involving long-lived sleptons.

Also, two additional minor modifications are introduced:

  1. The changes added in PR Fix in HepMC - G4 interface for displaced SUSY simulation #47165 are propagated to Generator3.cc.
  2. Flag "IsSmuon" is renamed to "IsSlepton", as it applies for selectrons as well.

PR validation:

This PR has been validated with signal samples of smuon pair production decaying to a muon and a gravitino (SMuon -> muon + G).

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:

This PR needs to be backported (together with #47165) to 10_6_X, 12_4_X and 13_0_X for Run2 and Run3 signal sample productions.

@civanch @kpedro88 @tvami @jaimeleonh

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 28, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @24LopezR for master.

It involves the following packages:

  • Configuration/ProcessModifiers (operations)
  • SimG4Core/Application (simulation)
  • SimG4Core/Generators (simulation)

@antoniovilela, @civanch, @cmsbuild, @davidlange6, @fabiocos, @kpedro88, @mandrenguyen, @mdhildreth, @rappoccio can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @bsunanda, @fabiocos, @makortel, @martinamalberti, @missirol, @rovere, @slomeo this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@kpedro88
Copy link
Contributor

please test

@civanch
Copy link
Contributor

civanch commented Jan 28, 2025

@24LopezR , is it really needed to add Configuration/ProcessModifiers - to me it is too much. I understand modifiers as a method to change parameters of some module for different era, for example, for run1 abd run2 something is completely different. Here we likely introduce a fix which is basically valid for all runs. We add as the default "False" and as custom "True" flags only to avoid instability in various analises, which we do not know. So, to me it is not era dependent parameter.

@kpedro88
Copy link
Contributor

It is useful to have a process modifier to make it easier for central MC requests to enable this feature. Otherwise, the cmsDriver commands require more in-depth customization, which is likely to be implemented incorrectly in McM (in my experience).

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 40KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7bec54/44004/summary.html
COMMIT: 629245f
CMSSW: CMSSW_15_0_X_2025-01-28-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47197/44004/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3994126
  • DQMHistoTests: Total failures: 87
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3994019
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 218 log files, 189 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented Jan 29, 2025

@kpedro88 , we have many parameters in g4SimHits configuration, I would not create a modifier for each. In this particular case it is a flag False/True. In the current variant of the PR, it seems to me that we simply put "True" as the default value for isSlepton flag. Earlier, when PR was more complicate, I proposed the default flag "False", but correct me if I am wrong: now "True" will work fine - let us check on couple of WFs.

@kpedro88
Copy link
Contributor

@civanch I agree that if the feature can be enabled by default without disrupting other workflows, that would be a preferable approach.

@24LopezR
Copy link
Contributor Author

@civanch @kpedro88 I also agree that enabling the flag by default shouldn't break other simulations. I am fine with doing it.

@tvami
Copy link
Contributor

tvami commented Jan 29, 2025

if the feature can be enabled by default without disrupting other workflows

I also agree with that, but I dont think this was proven yet, it would need high stats samples from PdmV. Tagging Sitian ( @DickyChant )

So I'd really be against changing the default for everybody!

I think there is precedent for using the proc modifier for these kind of bug fixes, for example https://github.com/cms-sw/cmssw/pull/36352/files

@civanch there is nothing wrong with the approach in this PR, right? I might be an overkill, but it's certainly the safest option to go with.

@civanch
Copy link
Contributor

civanch commented Jan 29, 2025

There is nothing wrong is this PR. if we set isSlepton=true, then for particles with pdg code 1000011, 1000013, 1000015, 2000011, 2000013, 2000015 we will have special (much more correct) treatment. Unfortunately, we have no chance to run many generators, where these particles are not present, or some are present. There is no test (set of tests), which gurantee absolute validity of such PR. What is feasible is to run current use-case with high statistics but it is aleardy done, @24LopezR ?

@tvami
Copy link
Contributor

tvami commented Jan 29, 2025

is to run current use-case with high statistics but it is aleardy done

Not for SM MCs, right?

@civanch
Copy link
Contributor

civanch commented Jan 29, 2025

Yes, the standard MC WFs like ttbar should not be affected at all.

@24LopezR
Copy link
Contributor Author

24LopezR commented Jan 30, 2025

@civanch I only tested this changes for signal MC samples of smuon->mu+G (with high statistics), but not for SM MC or any other workflow. I believe the changes should not affect any other simulation. In the worst case, suppressing the z cut can have an impact, but it affects only particles decaying in z>50m, certainly SM MC wouldn't be affected by this...

As @tvami says, the safest option is to have a process modifier just for our simulation. The other option is to set the IsSlepton flag to true by default and then wait for the next validation campaign to see the effects in many different scenarios, and then if something breaks up we revert the changes.

Both options are good in my opinion, but of course I'll let you decide.

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