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

Update Run 3 geometry DB payloads #31529

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

christopheralanwest
Copy link
Contributor

PR description:

This PR updates the geometry DB payloads in the Run 3 MC global tags. The changes and their validation are described in the 21 Sep 2020 AlCaDB presentation by @cvuosalo.

The Run 3 data GTs will be updated once release validation of the payloads has been completed.

The global tag diffs are the same for all six scenarios:

Attn: @cvuosalo @ianna

2021 design
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/112X_mcRun3_2021_design_v7/112X_mcRun3_2021_design_v8

2021 realistic
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/112X_mcRun3_2021_realistic_v7/112X_mcRun3_2021_realistic_v8

2021 cosmics
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/112X_mcRun3_2021cosmics_realistic_deco_v7/112X_mcRun3_2021cosmics_realistic_deco_v8

2021 heavy ion
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/112X_mcRun3_2021_realistic_HI_v8/112X_mcRun3_2021_realistic_HI_v9

2023 realistic
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/112X_mcRun3_2023_realistic_v7/112X_mcRun3_2023_realistic_v8

2024 realistic
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/112X_mcRun3_2024_realistic_v7/112X_mcRun3_2024_realistic_v8

PR validation:

Please see the 21 Sep 2020 AlCaDB presentation by @cvuosalo for details.

In addition, a technical test was performed: runTheMatrix.py -l limited,12024.0,7.23,159.0,12834.0,7.24 --ibeos

if this PR is a backport please specify the original PR and why you need to backport that PR:

This PR is not a backport and should not be backported.

@christopheralanwest
Copy link
Contributor Author

test parameters
workflow = 12024.0,7.23,159.0,12834.0,7.24

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31529/18505

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

Configuration/AlCa

@cmsbuild, @pohsun, @christopheralanwest, @tocheng, @tlampen can you please review it and eventually sign? Thanks.
@makortel, @Martin-Grunewald, @mmusich, @fabiocos, @tocheng this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@christopheralanwest
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 21, 2020

The tests are being triggered in jenkins.
Test Parameters:

@cmsbuild
Copy link
Contributor

+1
Tested at: d3c33ba
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e64cfa/9465/summary.html
CMSSW: CMSSW_11_2_X_2020-09-21-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e64cfa/9465/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e64cfa/12024.0_TTbar_13+2021Design+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e64cfa/159.0_HydjetQ_B12_5020GeV_2021_ppReco+HydjetQ_B12_5020GeV_2021_ppReco+DIGIHI2021PPRECO+RECOHI2021PPRECO+ALCARECOHI2021PPRECO+HARVESTHI2021PPRECO
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e64cfa/7.23_Cosmics_UP21+Cosmics_UP21+DIGICOS_UP21+RECOCOS_UP21+ALCACOS_UP21+HARVESTCOS_UP21
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-e64cfa/7.24_Cosmics_UP21_0T+Cosmics_UP21_0T+DIGICOS_UP21_0T+RECOCOS_UP21_0T+ALCACOS_UP21_0T+HARVESTCOS_UP21_0T

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 5776 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2540471
  • DQMHistoTests: Total failures: 144999
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2395450
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 149 log files, 22 edm output root files, 35 DQM output files

# GlobalTag for MC production with realistic conditions for Phase1 2021
'phase1_2021_realistic' : '112X_mcRun3_2021_realistic_v7', # GT containing realistic conditions for Phase1 2021
'phase1_2021_realistic' : '112X_mcRun3_2021_realistic_v8', # GT containing realistic conditions for Phase1 2021
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting to see also a new ExtentedZeroMaterial labeled tag for GeometryFileRcd, based on the Extended2021ZeroMaterial scenario introduced in #31082. Will that follow in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvuosalo - IMHO, these scenarios should be uploaded together. Otherwise, we should request to delete ExtentedZeroMaterial from this GT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the moment there is no such label in that GT:

$ conddb list 112X_mcRun3_2021_realistic_v8 | grep GeometryFileRcd
[2020-09-22 12:23:13,561] INFO: Connecting to pro [frontier://PromptProd/CMS_CONDITIONS]
GeometryFileRcd                                         CTPPS                                                          XMLFILE_CTPPS_Geometry_2018_101YV6                                                  
GeometryFileRcd                                         Extended                                                       XMLFILE_Geometry_112YV2_Extended2021_mc                                             
MFGeometryFileRcd                                       120812                                                         MFGeometry_120812                                                                   
MFGeometryFileRcd                                       130503                                                         MFGeometry_130503                                                                   
MFGeometryFileRcd                                       160812                                                         MFGeometry_160812                                                                   
MFGeometryFileRcd                                       90322                                                          MFGeometry_90322         

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a tag for the ExtendedZeroMaterial scenario exist? I understood from @cvuosalo comments at yesterday's AlCaDB meeting that it did not and that we should not wait for the zero material scenario to converge. Can the payload corresponding to #31082 be generated now or is additional work required?

@cvuosalo @ianna Please also see and respond to @fwyzard's recent HN post regarding use of old MC samples in releases built after this PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood from @cvuosalo comments at yesterday's AlCaDB meeting that it did not and that we should not wait for the zero material scenario to converge.

As far as I can see there are some samples for EGM that need that to fall in place in order to be injected [*], so some priority would be needed. Alternatively the driver command for those samples could be tweaked in order to deliver the simulation geometry from XML.

[*] https://hypernews.cern.ch/HyperNews/CMS/get/prep-ops/7302.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a tag for the ExtendedZeroMaterial scenario exist? I understood from @cvuosalo comments at yesterday's AlCaDB meeting that it did not and that we should not wait for the zero material scenario to converge. Can the payload corresponding to #31082 be generated now or is additional work required?

@cvuosalo @ianna Please also see and respond to @fwyzard's recent HN post regarding use of old MC samples in releases built after this PR is merged.

I'll check with @cvuosalo about the Zero material payload. The scripts used to produce all Sim geometry variations at once.

I've noticed there is a GeometryFileRcd with a CTPPS label. Is it with iovs? e.g. is it their Reco geometry? It should eventually go away, I think, they have decoupled their Reco geometry from the XML now.

The last point - the Magnetic field geometry. How old the payloads are? The change in Muon yoke thickness should be identical for both Sim geometry and MF geometry.

Sorry, I would have checked it myself, but I'm having trouble connecting to CondDB...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsusa Are the changes in #31082 sufficient to properly define the zero material scenario for Run 3? I (mis?)understood from your comments at yesterday's AlCa meeting that additional work was required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The real changes proposed in #31082 involve only the densities of the non-active volumes and IIUC are not coupled with the other (small) tracker changes (solution of overlaps, etc.) for Run-3. I would say the payload could be derived right away

@cvuosalo
Copy link
Contributor

Currently, before this PR, for GT 112X_mcRun3_2021_realistic_v7 there is no Zero Material Geometry File Record. This PR does not change that.
Creation of the Extended 2021 Zero Material Geometry File Record will require a small amount of development work. This PR is urgent, so I don't think the Zero Material scenario can be included yet.
@mmusich Would you like to create a GitHub issue to request the Extended 2021 Zero Material Geometry File Record? You could indicate in the issue the deadline for when the record is needed. If you cannot create the issue, I can do it.
Let us discuss the details in the issue.

@cvuosalo
Copy link
Contributor

Concerning the GeometryFileRcd with the "CTPPS" label, that is the reco geometry for PPS. The PPS group is still working on their reco geometry, so when it is finished, they could upload their revised reco geometry. Until then, I think we leave the record as is.

@cvuosalo
Copy link
Contributor

The MFGeometryFileRcd records are from 2014 and 2017. Nicola might want to reload them for Run 3.

@namapane FYI: The Run 3 Extended 2021 geometry is being uploaded to the Conditions DB. It contains minor changes in CSC geometry. You may want to upload new versions of the MFGeometryFileRcd for Run 3 when you think it necessary.

@cvuosalo
Copy link
Contributor

The comparison tests may be showing larger changes than might be expected in Run 3 workflows. The largest I've seen are in workflow 11634 where Tracker hits seem to move from one TOB to another in the "alternative comparisons". For example, [1] shows hits for the reference and none for the PR, while [2] shows hits for the PR and none for the reference. I'm not sure how to interpret these plots. In this workflow, the MC was re-generated with the new simulation geometry, so we expect to see some fluctuations just from the re-generation itself, but these changes seem larger than that.

@vargasa Could you please help to interpret what these plots tell us?

[1]:
image

[2]:
image

@cvuosalo
Copy link
Contributor

@vargasa Could you please comment on whether 10 MC events in the baseline and 10 independent MC events in the PR would be expected to produce the plots I show in my comment (#31529 (comment)), that is, certain slices of eta have no hits while other slices have thousands of hits?

@vargasa
Copy link
Contributor

vargasa commented Sep 24, 2020

@cvuosalo I see those differences not only within the outer barrel but also in end-cap, inner disks, etc. It may seem reasonable to see those differences when using different MC events. Is it possible to get more statistics (or perhaps rerun the workflows)? i.e check to see if the same regions remain empty (which may indicate there's a problem). @kpedro88, from previous PRs I have seen you working with these comparisons. Can you shine some light in here? please

@kpedro88
Copy link
Contributor

@vargasa currently there is no option for the bot to rerun comparisons with more statistics. If you want to make those comparisons, you have to run them yourself.

@cvuosalo
Copy link
Contributor

unhold

@cmsbuild cmsbuild removed the hold label Sep 24, 2020
@cvuosalo
Copy link
Contributor

The issues in #31519 go back to Run 2 and are not a reason to stop this PR, so I removed the hold.

@cvuosalo
Copy link
Contributor

I think the differences I posted #31529 (comment) are probably due to low statistics of comparing 10 events in the baseline to 10 independent events for the PR. I've generated 700 events each for baseline and PR geometry, and I will check them to see the differences.

@cvuosalo
Copy link
Contributor

With 700 MC events generated each for baseline and PR here are similar plots to those above.
I think the plots show there is substantial statistical fluctuation in these Tracker Hits plots, so differences should be expected.
I think it is reasonable to conclude that there is no evidence of problems in the Run 3 geometry payloads, so this PR should go ahead.

image

image

@vargasa
Copy link
Contributor

vargasa commented Sep 25, 2020

@cvuosalo Do you see any empty regions in the new plots?

@cvuosalo
Copy link
Contributor

@vargasa With 700 events, I didn't find any of these Tracker Hit plots that are empty for baseline or PR. They only show the kind of differences that I posted.

@cvuosalo
Copy link
Contributor

@christopheralanwest Can this PR go forward so that it gets into the pre-release on the 29th?

@davidlange6
Copy link
Contributor

davidlange6 commented Sep 25, 2020 via email

@cvuosalo
Copy link
Contributor

@davidlange6 Could you please suggest an easy way to get different random seeds? I'm getting the same events for 11634.0 when I do runTheMatrix.py twice.

@kpedro88
Copy link
Contributor

you can put this in the configs generated by runTheMatrix.py (replace 120 with the number of your choice):

from IOMC.RandomEngine.RandomServiceHelper import RandomNumberServiceHelper
randHelper = RandomNumberServiceHelper(process.RandomNumberGeneratorService)
randHelper.resetSeeds(120)

@cvuosalo
Copy link
Contributor

In a baseline vs. baseline comparison (both using the current Global Tag) with 10 events each with different random seeds, the plots show similar statistical fluctuations as seen in the PR comparison tests. I think we can conclude that the comparison differences in the PR tests are due to fluctuations from the low statistics (only 10 events) and not due to any error in the new geometry.

The black in the plots below is the same standard 10 events for 11634.0 with the same random seed as in the PR test. The red is 10 events with a different random seed.

image
image

@christopheralanwest
Copy link
Contributor Author

@cvuosalo Are you satisfied with your checks so that I may sign off on this PR? If the zero material payload becomes available, I can open a new PR to add it. I would rather not delay this PR unnecessarily so as to ensure that it goes into the next pre-release.

@cvuosalo
Copy link
Contributor

@christopheralanwest Yes, I am satisfied. Please approve this PR so it can be merged and get into pre7. The zero material scenario is not urgent and can come later.

@christopheralanwest
Copy link
Contributor Author

+alca

@cmsbuild
Copy link
Contributor

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

@qliphy
Copy link
Contributor

qliphy commented Sep 29, 2020

+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