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

Run3-gex168 Modify all Run3 scenarios after fixing the HCAL SD issue #42476

Merged
merged 1 commit into from
Aug 6, 2023

Conversation

bsunanda
Copy link
Contributor

@bsunanda bsunanda commented Aug 5, 2023

PR description:

Modify all Run3 scenarios after fixing the HCAL Sensitive DEtector (SD) issue. The dd4hep was using some additional SDs which need not be in the list producing hits in HCAL. This was corrected in #42463 and these corrections are incorporated in this list. If this causes a serious impact on the simulation, it clearly indicates that this PR needs to be back ported to 13_2_X, 13_1_X, 13_0_X

PR validation:

Use the runTheMatrix test workflows

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:

Backport to be decided later

@bsunanda
Copy link
Contributor Author

bsunanda commented Aug 5, 2023

@cmsbuild Please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42476/36484

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2023

A new Pull Request was created by @bsunanda (Sunanda Banerjee) for master.

It involves the following packages:

  • Configuration/Geometry (geometry, upgrade)
  • Geometry/CMSCommonData (geometry, upgrade)

@civanch, @Dr15Jones, @bsunanda, @makortel, @mdhildreth, @AdrianoDee, @srimanob can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @fabiocos, @slomeo, @vargasa 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

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 5, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-23ff76/34101/summary.html
COMMIT: 71da4e3
CMSSW: CMSSW_13_3_X_2023-08-05-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42476/34101/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 7 lines to the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 2901 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3150821
  • DQMHistoTests: Total failures: 6188
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3144611
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@bsunanda
Copy link
Contributor Author

bsunanda commented Aug 6, 2023

+geometry

@bsunanda
Copy link
Contributor Author

bsunanda commented Aug 6, 2023

@srimanob Please approve this

@perrotta
Copy link
Contributor

perrotta commented Aug 6, 2023

@bsunanda I imagine you refere to the new geometries xml files integrated with #42463
Please, write in the PR description of this PR:

  • what was the issue fixed by those modified geometry files
  • what differences are you expecting in output, so that one can judge the large number of differences observed in the tests

@srimanob
Copy link
Contributor

srimanob commented Aug 6, 2023

Hi @bsunanda @perrotta

I would propose, for Run-3, after the PR is merged to master, we ask PdmV to look on impact of the bug and decide if we should backport or not. Backporting will create inconsistent in the production release. I think we should do it only we understand the impact, and we would like to restart the production. If the impact is very small, I think the bug-fix should go only 2024, and legacy of Run-3.

By the way, should new geometry be created as payload for GT? @cms-sw/alca-l2
If yes, maybe we should do it before the 13_3_0_pre2.

Thx.
FYI @cms-sw/pdmv-l2

@srimanob
Copy link
Contributor

srimanob commented Aug 6, 2023

+Upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 6, 2023

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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

perrotta commented Aug 6, 2023

The differences are rather large in the test outputs, but they could be even due to the reshuffling of the random seeds, Definitely a validation is necessary to assess the size of the changes, and it could be done in pre2

Unless there are such macroscopic differences that would require to start once again the simulations with 13_0 (Run3) and 13_1 (Phase2), I wouldn't backport into those cycles. I would rather prepare a backport in 13_2 for the HI run and MC.

By the way, @bsunanda : ahem, what is a "SD"? Differences between the xml files after and before the fix are as:

 diff v2/hcalRecNumbering.xml v1/hcalRecNumbering.xml
21,23c21
<     <PartSelector path="//HBS.*"/>
<     <PartSelector path="//HTS.*"/>
<     <PartSelector path="//HVQX"/>
---
>     <PartSelector path="//HCal"/

@civanch
Copy link
Contributor

civanch commented Aug 6, 2023

@perrotta , SD is SensitiveDetector. The problem fixed in this PR does not change geometry itself but change names and list of SD labels assigned to geometry volumes. My personal impression that backports to 3_2 and 3_1 are safe, because no SIM production in these releases. Backport to 3_0 should be done when we understand consequences of this fix. So, @bsunanda , please, prepare backports. Let us see what happens in 13_3_0_pre2 and after decide on backport.

@perrotta
Copy link
Contributor

perrotta commented Aug 6, 2023

+1

  • Validation with CMSSW_13_3_0_pre2 is expected

@cmsbuild cmsbuild merged commit 32826b2 into cms-sw:master Aug 6, 2023
@srimanob
Copy link
Contributor

srimanob commented Aug 6, 2023

@perrotta , SD is SensitiveDetector. The problem fixed in this PR does not change geometry itself but change names and list of SD labels assigned to geometry volumes. My personal impression that backports to 3_2 and 3_1 are safe, because no SIM production in these releases. Backport to 3_0 should be done when we understand consequences of this fix. So, @bsunanda , please, prepare backports. Let us see what happens in 13_3_0_pre2 and after decide on backport.

Hi Vladimir, 13_1 is used for Phase-2 where production is almost done. I would avoid to backport to 13_1.

@perrotta
Copy link
Contributor

perrotta commented Aug 6, 2023

Just to be clear: backports to 13_0 and 13_1 will be accepted only in the "worst case scenario", if we realize that the already started simulation is just rubbish,
Obviously, nobody want to do so!

@bsunanda
Copy link
Contributor Author

bsunanda commented Aug 7, 2023 via email

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