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

Backport (update) option to override GEN decays with FastSim decays #41399

Closed

Conversation

sbein
Copy link
Contributor

@sbein sbein commented Apr 25, 2023

PR description:

The backport #39022 was missing two key lines that were in the HEAD, which this PR corrects for. These changes are needed to achieve better agreement between FastSim and FullSim for Standard Model production, e.g., ttbar, for Run 2 UL productions.

PR validation:

@cmsbuild
Copy link
Contributor

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

@sbein
Copy link
Contributor Author

sbein commented Apr 25, 2023

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-aade37/32124/summary.html
COMMIT: 243f2c4
CMSSW: CMSSW_10_6_X_2023-04-23-0000/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41399/32124/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 14 lines from the logs
  • Reco comparison results: 2915 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 3215686
  • DQMHistoTests: Total failures: 5772
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3209580
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 143 log files, 102 edm output root files, 35 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented Apr 25, 2023

+1

@cmsbuild
Copy link
Contributor

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_1_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)

@civanch
Copy link
Contributor

civanch commented Apr 25, 2023

Regression problems are only in FastSim WFs that is expected.

@perrotta
Copy link
Contributor

While the fix absolutely makes sense, still we are updating a closed release with some fix that changes the fastsim output: shouldn't we apply a modifier then to identify the fastsim productions made with and without this fix?

@sbein
Copy link
Contributor Author

sbein commented Apr 25, 2023

@perrotta I think you're right, but it gets a little complicated. This fix supersedes a previous bug fix involving the decayer, e.g., #36352, which got its own proc modifier. That fix was kind of a hack that solved a specific issue for exotic decays, but later issues were discovered with the decayer even in SM events, which came to light when studying FastSim for use cases beyond the SUS PAG. We decided to just disable the decayer by default since it causes more trouble than it's worth, rather than have bug fixes on top of previous bug fixes.

These changes don't make a difference with respect to current SUS campaigns, but they will be necessary if TOP decides to make requests. In my opinion, it would serve to cut a bit of red tape here, but we also need to work closely with the PAGs as FastSim becomes more used by more than one PAG. My two cents, thanks a lot.

@perrotta
Copy link
Contributor

These changes don't make a difference with respect to current SUS campaigns, but they will be necessary if TOP decides to make requests. In my opinion, it would serve to cut a bit of red tape here, but we also need to work closely with the PAGs as FastSim becomes more used by more than one PAG. My two cents, thanks a lot.

Really SUS events are untouched by this change? In the test matrix there are only ZEE and TOP FS workflows, therefore this cannot be verified: it would be worth testing with a couple of SUSY FASTSIM workflow to verify. Could you please suggest a couple of them so that we can run the tests with them?

On the other hand, if that is true and FastSim in 10_6_X was not used so far for any non SUSY campaign (including MB backgrounds), then I have no objections in merging this without a modifier: could please @cms-sw/pdmv-l2 remind us here which FastSim campaigns were run so far in the legacy 10_6_X?

@rappoccio
Copy link
Contributor

hold

  • Explicitly holding until the questions from @perrotta are answered.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2023

Pull request has been put on hold by @rappoccio
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

@sbein
Copy link
Contributor Author

sbein commented May 9, 2023

Indeed the only campaigns so far were SUSY signal scans. However, to preserve default physics, the original fixLongLivedBug modifier is restored and the option to ignore all FastSim decays in place of the GEN decays is made as an option activated with a proc modifer. This PR is superseded by: #41596

@sbein sbein closed this May 9, 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