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

PPS: update of era modifiers #33250

Closed
wants to merge 3 commits into from
Closed

Conversation

jan-kaspar
Copy link
Contributor

PR description:

This PR proposes a tiny reorganisation of (CT)PPS era modifiers in order to avoid mistakes and provide flexibility needed for Run 3. The motivation and the selected solution are described in #33080 and PPS SW meeting presentation on 10 Mar 2021. In brief, the changes include:

  • new era modifier ctpps introduced - turned on for all eras where PPS was/is active
  • the meaning of ctpps_2016 is restricted to 2016 only

This update shall be transparent whenever Run2_20XY or Run3 modifier chains are used.

This is a technical update, no change in test results expected.

No backport is foreseen.

PR validation:

Below, two comparisons before this PR (blue) vs. after the PR (red dashed) are attached:

No change observed, as expected.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33250/21712

  • This PR adds an extra 52KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @jan-kaspar for master.

It involves the following packages:

Configuration/Eras
Configuration/EventContent
Configuration/StandardSequences
DQM/CTPPS
PhysicsTools/PatAlgos
RecoPPS/Configuration
RecoPPS/Local
RecoPPS/ProtonReconstruction

@perrotta, @andrius-k, @kmaeshima, @ErnestaP, @ahmad3213, @cmsbuild, @silviodonato, @franzoni, @jfernan2, @slava77, @jpata, @qliphy, @rvenditti, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @felicepantaleo, @forthommel, @hatakeyamak, @emilbols, @Martin-Grunewald, @mbluj, @ahinzmann, @seemasharmafnal, @mmarionncern, @makortel, @smoortga, @dgulhan, @jdolen, @slomeo, @ferencek, @GiacomoSguazzoni, @rovere, @VinInn, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @mtosi, @fabiocos, @clelange, @swozniewski, @JyothsnaKomaragiri, @lecriste, @gpetruc, @mariadalfonso, @andrzejnovak 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

@slava77
Copy link
Contributor

slava77 commented Mar 23, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4be71c/13692/summary.html
COMMIT: 518d281
CMSSW: CMSSW_11_3_X_2021-03-22-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33250/13692/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

----- Begin Fatal Exception 23-Mar-2021 14:10:33 CET-----------------------
An exception of category 'CTPPSGeometry' occurred while
   [0] Processing global begin Run run: 1
   [1] Calling method for module CTPPSDiamondDQMSource/'ctppsDiamondDQMSource'
Exception Message:
Not found detector with ID 2054160384, i.e. subDet=5 arm=0 station=1 rp=6
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 23-Mar-2021 14:22:38 CET-----------------------
An exception of category 'CTPPSGeometry' occurred while
   [0] Processing global begin Run run: 1
   [1] Calling method for module CTPPSDiamondDQMSource/'ctppsDiamondDQMSource'
Exception Message:
Not found detector with ID 2054160384, i.e. subDet=5 arm=0 station=1 rp=6
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 23-Mar-2021 14:24:20 CET-----------------------
An exception of category 'CTPPSGeometry' occurred while
   [0] Processing global begin Run run: 1
   [1] Calling method for module CTPPSDiamondDQMSource/'ctppsDiamondDQMSource'
Exception Message:
Not found detector with ID 2054160384, i.e. subDet=5 arm=0 station=1 rp=6
----- End Fatal Exception -------------------------------------------------

AddOn Tests

----- Begin Fatal Exception 23-Mar-2021 14:05:59 CET-----------------------
An exception of category 'CTPPSGeometry' occurred while
   [0] Processing  Event run: 323775 lumi: 99 event: 110936866 stream: 1
   [1] Running path 'FEVTDEBUGHLToutput_step'
   [2] Prefetching for module PoolOutputModule/'FEVTDEBUGHLToutput'
   [3] Prefetching for module CTPPSDiamondLocalTrackFitter/'ctppsDiamondLocalTracks'
   [4] Calling method for module CTPPSDiamondRecHitProducer/'ctppsDiamondRecHits'
Exception Message:
Not found detector with ID 2054160384, i.e. subDet=5 arm=0 station=1 rp=6
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 23-Mar-2021 14:05:51 CET-----------------------
An exception of category 'CTPPSGeometry' occurred while
   [0] Processing  Event run: 325112 lumi: 6 event: 25520 stream: 2
   [1] Running path 'FEVTDEBUGHLToutput_step'
   [2] Prefetching for module PoolOutputModule/'FEVTDEBUGHLToutput'
   [3] Prefetching for module CTPPSDiamondLocalTrackFitter/'ctppsDiamondLocalTracks'
   [4] Calling method for module CTPPSDiamondRecHitProducer/'ctppsDiamondRecHits'
Exception Message:
Not found detector with ID 2054160384, i.e. subDet=5 arm=0 station=1 rp=6
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 23-Mar-2021 14:05:44 CET-----------------------
An exception of category 'CTPPSGeometry' occurred while
   [0] Processing  Event run: 323775 lumi: 99 event: 111372621 stream: 0
   [1] Running path 'FEVTDEBUGHLToutput_step'
   [2] Prefetching for module PoolOutputModule/'FEVTDEBUGHLToutput'
   [3] Prefetching for module CTPPSDiamondLocalTrackFitter/'ctppsDiamondLocalTracks'
   [4] Calling method for module CTPPSDiamondRecHitProducer/'ctppsDiamondRecHits'
Exception Message:
Not found detector with ID 2054160384, i.e. subDet=5 arm=0 station=1 rp=6
----- End Fatal Exception -------------------------------------------------
Expand to see more addon errors ...

@jan-kaspar
Copy link
Contributor Author

I've analysed most of the failing tests from #33250 (comment). All of them have Run3 era declared.

As far as I understand, the test failures are not due to a bug in this PR, but due to the fact that this PR fixes a bug which prevented observing the problem earlier. It has to do with this line:
https://github.com/cms-sw/cmssw/blob/master/Geometry/VeryForwardGeometry/python/geometryRPFromDB_cfi.py#L19
Up to now, by mistake, isRun2 was set to True even if Run3 era was declared. This is now fixed and it exposes the upstream problem - the Run3 geometry which is now in GT(s) is in fact still the old Run2 geometry. Because of the differences introduced in #31484, parsing Run2 geometry with isRun2=false yields problems, namely the diamond detectors are assigned wrong ids - and this leads to the exception seen in the logs.

This has not been intercepted by my tests since they use the geometry loaded from XML files - and this is correct.

As far as I understand, we need urgently to upload the Run3 (diamond) geometry to DB, otherwise we are stuck.

@forthommel @wpcarvalho @clemencia : can you please double check my assessment and, if correct, could you please update the Run3 DB and GTs (to be compatible with the XML files in the CMSSW code)?

@fabferro FYI

@makortel
Copy link
Contributor

As far as I understand, we need urgently to upload the Run3 (diamond) geometry to DB, otherwise we are stuck.

FYI @cms-sw/geometry-l2

@silviodonato
Copy link
Contributor

FYI @cms-sw/alca-l2

@perrotta
Copy link
Contributor

@jan-kaspar why the comparisons for the "direct simulation" posted in the PR description do not show differences in the 2021 case? If I understand correctly your explanation for the origin of the failing tests, without this PR you should have picked the run2 geometry for that test, while with this PR the correct run3 geometry (in your case, as taken from the xml file) should have been used instead.

(By the way, for the "reconstruction" case there is no "2021" run3 comparison in your plots)

@jan-kaspar
Copy link
Contributor Author

@jan-kaspar why the comparisons for the "direct simulation" posted in the PR description do not show differences in the 2021 case? If I understand correctly your explanation for the origin of the failing tests, without this PR you should have picked the run2 geometry for that test, while with this PR the correct run3 geometry (in your case, as taken from the xml file) should have been used instead.

Unlike other WFs, the "direct" simulation loads the geometry from XML files (where the Run3 geometry is already correct):
https://github.com/cms-sw/cmssw/blob/master/Validation/CTPPS/python/simu_config/year_2021_cff.py#L38

(By the way, for the "reconstruction" case there is no "2021" run3 comparison in your plots)

This comparison may be better called "LHC data reco" comparison as it is run on real LHC data. Obviously, they do not yet exist for Run3, thus they are missing there.

@wpcarvalho
Copy link
Contributor

@forthommel @wpcarvalho @clemencia : can you please double check my assessment and, if correct, could you please update the Run3 DB and GTs (to be compatible with the XML files in the CMSSW code)?

@fabferro FYI

I started looking at the problem and hope we can provide a solution soon.

@civanch
Copy link
Contributor

civanch commented Apr 6, 2021

@cvuosalo , in the new Run-3 geometry we have also diamond detectors or not yet?

@cvuosalo
Copy link
Contributor

cvuosalo commented Apr 6, 2021

@civanch The Run 3 simulation geometry for 11_3 contains a PPS diamond detector assembly.

@perrotta
Copy link
Contributor

perrotta commented Apr 8, 2021

@jan-kaspar @wpcarvalho : having the updated PPS run3 geometry in the GT goes beyond the limited scope of this PR.
Next week, ~ on Tuesday, is the deadline for the last open pre-release of 11_3, and it would be a pity if that geometry cannot be loaded by then. Do you have this schedule in mind for your update? I think that the integration of this PR could follow shortly after it.

@jan-kaspar
Copy link
Contributor Author

@jan-kaspar @wpcarvalho : having the updated PPS run3 geometry in the GT goes beyond the limited scope of this PR.
Next week, ~ on Tuesday, is the deadline for the last open pre-release of 11_3, and it would be a pity if that geometry cannot be loaded by then. Do you have this schedule in mind for your update? I think that the integration of this PR could follow shortly after it.

I agree, updating the geometry is conceptually unrelated to this PR. This PR just unveils a problem which should have been fixed earlier. This problem now blocks this PR and other PPS developments and therefore, I agree, the geometry problem should be addressed quasi immediately. We discussed it in the PPS SW meeting yesterday and indeed we gave this fix the highest priority.
@wpcarvalho has made progress in providing a solution. I let him confirm the timeline, but I hope to be on time for the Tuesday deadline. I don't mind if this PR delayed because of this.

@wpcarvalho
Copy link
Contributor

Sorry for not replying earlier, for some unknown reason the GitHub notifications started to go to my Spam folder.
I have build a geometry file which may fix the problem but have to check if it is correctly done and if any validation is needed. I will contact @cvuosalo in a separate thread to clarify this issue.

@perrotta
Copy link
Contributor

test parameters:

workflow = 11725.0,11925.0
pull_request = #33404

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: UnitTests AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4be71c/14182/summary.html
COMMIT: 518d281
CMSSW: CMSSW_11_3_X_2021-04-11-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33250/14182/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test TestDQMOfflineConfiguration150 had ERRORS
---> test TestDQMOfflineConfiguration50 had ERRORS

AddOn Tests

----- Begin Fatal Exception 12-Apr-2021 17:48:27 CEST-----------------------
An exception of category 'CTPPSGeometry' occurred while
   [0] Processing  Event run: 323775 lumi: 99 event: 111190435 stream: 2
   [1] Running path 'FEVTDEBUGHLToutput_step'
   [2] Prefetching for module PoolOutputModule/'FEVTDEBUGHLToutput'
   [3] Prefetching for module CTPPSDiamondLocalTrackFitter/'ctppsDiamondLocalTracks'
   [4] Calling method for module CTPPSDiamondRecHitProducer/'ctppsDiamondRecHits'
Exception Message:
Not found detector with ID 2054160384, i.e. subDet=5 arm=0 station=1 rp=6
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 12-Apr-2021 17:48:32 CEST-----------------------
An exception of category 'CTPPSGeometry' occurred while
   [0] Processing  Event run: 325112 lumi: 6 event: 25760 stream: 3
   [1] Running path 'FEVTDEBUGHLToutput_step'
   [2] Prefetching for module PoolOutputModule/'FEVTDEBUGHLToutput'
   [3] Prefetching for module CTPPSDiamondLocalTrackFitter/'ctppsDiamondLocalTracks'
   [4] Calling method for module CTPPSDiamondRecHitProducer/'ctppsDiamondRecHits'
Exception Message:
Not found detector with ID 2054160384, i.e. subDet=5 arm=0 station=1 rp=6
----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 12-Apr-2021 17:48:15 CEST-----------------------
An exception of category 'CTPPSGeometry' occurred while
   [0] Processing  Event run: 323775 lumi: 99 event: 111372621 stream: 1
   [1] Running path 'FEVTDEBUGHLToutput_step'
   [2] Prefetching for module PoolOutputModule/'FEVTDEBUGHLToutput'
   [3] Prefetching for module CTPPSDiamondLocalTrackFitter/'ctppsDiamondLocalTracks'
   [4] Calling method for module CTPPSDiamondRecHitProducer/'ctppsDiamondRecHits'
Exception Message:
Not found detector with ID 2054160384, i.e. subDet=5 arm=0 station=1 rp=6
----- End Fatal Exception -------------------------------------------------
Expand to see more addon errors ...

Comparison Summary

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

@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-4be71c/11725.0_GluGluTo2Jets_14TeV+2021+GluGluTo2Jets_M_300_2000_14TeV_Exhume_GenSim+Digi+Reco+HARVEST+ALCA
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4be71c/11925.0_GluGluTo2Jets_14TeV+2021PU+GluGluTo2Jets_M_300_2000_14TeV_Exhume_GenSim+DigiPU+RecoPU+HARVESTPU+Nano

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1329 differences found in the comparisons
  • DQMHistoTests: Total files compared: 38
  • DQMHistoTests: Total histograms compared: 2865506
  • DQMHistoTests: Total failures: 3703
  • DQMHistoTests: Total nulls: 20
  • DQMHistoTests: Total successes: 2861761
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 45.699 KiB( 37 files compared)
  • DQMHistoSizes: changed ( 140.53 ): 44.531 KiB Hcal/DigiRunHarvesting
  • DQMHistoSizes: changed ( 140.53 ): 1.172 KiB RPC/DCSInfo
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 160 log files, 37 edm output root files, 38 DQM output files
  • TriggerResults: no differences found

@silviodonato
Copy link
Contributor

This PR has been included in #33415

@perrotta
Copy link
Contributor

-1
(Now included in #33415, please close this PR)

@jan-kaspar
Copy link
Contributor Author

Closing as included in #33415

@jan-kaspar jan-kaspar closed this Apr 13, 2021
@jan-kaspar jan-kaspar deleted the pps_era_modifiers branch August 31, 2021 08:26
@jfernan2 jfernan2 mentioned this pull request Oct 1, 2021
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

9 participants