CTPPS update Run3 Direct Simulation to use old LHCInfo after LHCInfo update #42890

Glitchmin · 2023-09-27T14:53:55Z

PR description:

A followup PR to #42515, this PR updates Direct Simulation to be able to use both old LHCInfo and new LHCInfoPer* records for possible future updates.
Moreover it introduces a new Process Modifier Run3_CTPPS_directSim to be used when Direct Simulation is ran in Run3. This is needed because this simulation uses old LHCInfo no matter the run number. (In every other case modules ran in Run3 should be looking for new LHCInfoPer* records)

PR validation:

To validate please run Validation/CTPPS/test/simu/run_multiple

cmsbuild · 2023-09-27T15:00:18Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42890/37026

This PR adds an extra 64KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoPPS/ProtonReconstruction/plugins/CTPPSProtonProducer.cc modified in PR(s): PPS proton reco update to use new LHCInfoPer* records #39686
- File RecoPPS/ProtonReconstruction/python/ctppsProtons_cff.py modified in PR(s): PPS proton reco update to use new LHCInfoPer* records #39686

cmsbuild · 2023-09-27T15:00:41Z

A new Pull Request was created by @Glitchmin (Adam Kulczycki) for master.

It involves the following packages:

CalibPPS/ESProducers (alca)
CondFormats/DataRecord (db, alca)
CondTools/RunInfo (db)
Configuration/Eras (operations)
RecoPPS/ProtonReconstruction (reconstruction)
SimPPS/DirectSimProducer (simulation)
SimTransport/PPSProtonTransport (simulation)
Validation/CTPPS (dqm)

@davidlange6, @nothingface0, @fabiocos, @antoniovagnerini, @tjavaid, @francescobrivio, @cmsbuild, @mandrenguyen, @mdhildreth, @emanueleusai, @rappoccio, @saumyaphor4252, @syuvivida, @perrotta, @civanch, @jfernan2, @antoniovilela, @rvenditti, @consuegs can you please review it and eventually sign? Thanks.
@forthommel, @seemasharmafnal, @fabiocos, @fabferro, @grzanka, @mmusich, @makortel, @tocheng, @missirol, @Martin-Grunewald, @AnnikaStein this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

AndreaBellora · 2023-09-28T09:12:59Z

Configuration/Eras/python/Modifier_run3_directSim_cff.py

@@ -0,0 +1,3 @@
+import FWCore.ParameterSet.Config as cms
+
+run3_directSim =  cms.Modifier()


run3_directSim might not be a nice name. We should have the same behaviour also in Run 2 (or anyway make it clear that Run 2 works in the same way).

Hmm, that might be a good suggestion, if I understand correctly I can just name it run_direct_sim and it will be setting useNewLHCInfo to false

I would also add "pps" to the name of the modifier

I would also add "pps" to the name of the modifier

Using ctpps name would be more consistent with the existing modifiers.

Yup, I believe that something like ctpps_directSim could be more appropriate, as we don't really distinguish between Run 2 and Run 3 for what concerns the usage of LHCInfo in the PPS direct simulation

Changed in 8617402

AndreaBellora · 2023-09-28T09:15:57Z

Validation/CTPPS/plugins/CTPPSHepMCDistributionPlotter.cc

 void CTPPSHepMCDistributionPlotter::analyze(const edm::Event &iEvent, const edm::EventSetup &iSetup) {
  // get conditions
-  const auto &lhcInfo = iSetup.getData(lhcInfoESToken_);
+  LHCInfoCombined lhcInfoCombined(
+      iSetup, lhcInfoPerLSToken_, lhcInfoPerFillToken_, lhcInfoToken_, useNewLHCInfo_, iEvent.isRealData());


This module is plotting HepMC stuff, so it's clearly running on MC. It won't even run on top of real data, so we can hard-code the usage of the old LHCInfo

Well it didn't cost me much time to change it and now it offers some extra stuff like isCrossingAngleValid etc and maybe - that's actually something I don't know - Direct Simulation will one day be using new LHCInfoPer* records?

I don't think so, because the simulation should be setup via manual configurations. It's hard to say if run-dependent simulation will ever be used for PPS, but I'd exclude it as an option for now

AndreaBellora · 2023-09-28T09:17:42Z

CondTools/RunInfo/interface/LHCInfoCombined.h


  void setFromLHCInfo(const LHCInfo& lhcInfo);
  void setFromPerLS(const LHCInfoPerLS& infoPerLS);
  void setFromPerFill(const LHCInfoPerFill& infoPerFill);

-  float crossingAngle();
+  static inline bool useNewLHCInfoForDirectSimu = false;


Do I understand correctly, this is a temporary way to switch between one mechanism and the other, right?
If so, it should be removed before the final PR. Also, I believe this should be const 😄

It should be const
If isRealData()-based selection mechanism will make it to the final PR this bool should be here unless we're 100% sure Direct Simulation will never be using new LHCInfoPer*

makortel · 2023-09-28T13:17:38Z

Configuration/Eras/python/Era_Run3_CTPPS_directSim_cff.py

+from Configuration.Eras.Era_Run3_cff import Run3
+from Configuration.Eras.Modifier_run3_directSim_cff import run3_directSim
+
+Run3_CTPPS_directSim = cms.ModifierChain(Run3,run3_directSim)


If these Era+Modifier are not about data taking conditions, it would be better to move the modifier to Configuration/ProcessModifiers/python.

makortel · 2023-09-28T13:19:00Z

Validation/CTPPS/python/ctppsHepMCDistributionPlotter_cff.py

+
+
+from Configuration.Eras.Modifier_run3_common_cff import run3_common
+run3_common.toModify(CTPPSHepMCDistributionPlotter, useNewLHCInfo = True)


Maybe ctpps_2022 Modifier would be better? The run3_common should not be used for subdetector-specific customizations.

BTW, shouldn't this be run3_directSim or whatever the final name will be?

@makortel Using run3_common was decided on a AlCaDB meeting with Tamas Vami and Francesco Brivio, where we explicitly discussed which modifier to use. run3_common is also used in a already closed PR #42515
@AndreaBellora Please notice that I'm using 2 different modifiers run3_common to set useNewLHCInfo to True and run3_directSim to set useNewLHCInfo to False again. One is used every time we're in a Run3, second one is used to revert setting useNewLHCInfo for DirectSim

@makortel Using run3_common was decided on a AlCaDB meeting with Tamas Vami and Francesco Brivio, where we explicitly discussed which modifier to use. run3_common is also used in a already closed PR #42515

@Glitchmin @tvami @francescobrivio could you please remind me which was the rationale of using run3_common instead of ctpps_2022? run3_common is built including ctpps_2022 nonetheless, therefore the final effect should be the same. Using run3_common could be preferred in case a possible ctpps_2024 (for example) will fully replace ctpps_2022 (as it was the case for ctpps_2022 that fully replaced previous ctpps_2018 for run3): of course, in that case for future runN eras one would need to customize that ctpps_2024 explicitly, while by customizing run3_common instead this setting will be propagated till all future runs, including Phase2 ones.

@perrotta the point was that LHCInfoPer* records are used in Run-3, and we don't plan to go back to Run-2/Run-1 with them (this is usually not the case when a new record is introduced). So putting that older PR under run3_common was very natural. Not sure what the (far) future plans (for a rereco) are for 900 GeV runs in 2021, but the LHCInfoPer* records should be populated there too, so using ctpps_2022 might be misleading there. Otherwise probably ctpps_2022 is good too.

@tvami ctpps_2022 is actually used to define the whole Run3 era, see Era_Run3_cff.py: even in case of a possible rerun of 2021 data, that modifier will be picked by that era, in spite of having "2022" in its name. I agree that it would have been better to name ctpps_2022 something like run3_ctpps since the beginning (as for the other subdetectors and subpackages): but here we are.

Therefore, for the modifications themselves using ctpps_2022 or run3_common is completely equivalent. I tend to agree with @makortel that the "run3_common should not be used for subdetector-specific customizations". Beside that, I have not a strong opinion on that, and I wouldn't consider it a showstopper, unless @cms-sw/core-l2 people keep suggesting to enforce is.

sounds good to me!

The design intent, that has also been mostly followed (but not everywhere, and not enforced), was that we create a Modifier for these "subdetector-specific" customizations, and then construct the Eras (technically ModifierChains) from these Modifiers kind of like building a house from lego bricks.

I realize now that the idea seems to be to change the default for Run3 and beyond to something, and then add another modifier to be able to switch the behavior back (to Run2 and earlier). If the customizations that are currently tied to run3_common would be done with another modifier, say run3_ctpps_sim (just for the example, please invent a better name), the Run3_CTPPS_directSim Era could be defined as

Run3_CTPPS_directSim = Run3.copyAndExclude([run3_ctpps_sim])

To me this approach would be cleaner than customizing things back and forth.

cmsbuild · 2023-10-31T17:36:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42890/37464

This PR adds an extra 36KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoPPS/ProtonReconstruction/python/ctppsProtons_cff.py modified in PR(s): PPS proton reco update to use new LHCInfoPer* records #39686

cmsbuild · 2023-10-31T17:36:50Z

Pull request #42890 was updated. @davidlange6, @saumyaphor4252, @fabiocos, @jfernan2, @tjavaid, @francescobrivio, @civanch, @mdhildreth, @rvenditti, @nothingface0, @cmsbuild, @perrotta, @rappoccio, @antoniovilela, @syuvivida, @mandrenguyen, @antoniovagnerini, @consuegs can you please check and sign again.

smuzaffar · 2023-11-06T16:50:51Z

Milestone for this pull request has been moved to CMSSW_14_0_X.Please open a backport if it should also go in to CMSSW_13_3_X.

tjavaid · 2023-12-18T14:52:27Z

@Glitchmin, @perrotta , we do not observe the bin-by-bin comparison histograms to review before we could sign. There is an open issue #43590 being discussed which is hoped to be fixed soon.

civanch · 2023-12-19T07:54:40Z

+1

perrotta · 2023-12-19T16:16:49Z

please test

cmsbuild · 2023-12-19T19:16:06Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a9ac7a/36580/summary.html
COMMIT: e224e1e
CMSSW: CMSSW_14_0_X_2023-12-19-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42890/36580/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 87 lines from the logs
Reco comparison results: 22 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3429858
DQMHistoTests: Total failures: 1216
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3428620
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 214 log files, 167 edm output root files, 50 DQM output files
TriggerResults: no differences found

antoniovilela · 2023-12-20T21:33:37Z

@cms-sw/dqm-l2 @cms-sw/alca-l2
Do things look ok with this PR?
Thanks.

tjavaid · 2023-12-21T10:21:44Z

+1

spurious differences observed from unstable WF 12434.7 (open issue spurious differences in outputs of Run-3 wfs *.7 #39803 )

antoniovilela · 2023-12-21T11:37:10Z

+1

cmsbuild · 2023-12-21T11:37:32Z

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 be automatically merged.

cmsbuild added this to the CMSSW_13_3_X milestone Sep 27, 2023

cmsbuild added reconstruction-pending simulation-pending dqm-pending alca-pending db-pending operations-pending pending-signatures tests-pending orp-pending code-checks-pending labels Sep 27, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 27, 2023

Glitchmin changed the title ~~update Run3 Direct Simulation to use old LHCInfo after LHCInfo update~~ CTPPS update Run3 Direct Simulation to use old LHCInfo after LHCInfo update Sep 27, 2023

AndreaBellora reviewed Sep 28, 2023

View reviewed changes

makortel reviewed Sep 28, 2023

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 31, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 31, 2023

smuzaffar modified the milestones: CMSSW_13_3_X, CMSSW_14_0_X Nov 6, 2023

cmsbuild modified the milestones: CMSSW_14_0_X, CMSSW_13_3_X Nov 6, 2023

cmsbuild added simulation-approved and removed simulation-pending labels Dec 19, 2023

cmsbuild added tests-started and removed tests-approved labels Dec 19, 2023

cmsbuild added tests-approved and removed tests-started labels Dec 19, 2023

cmsbuild added dqm-approved and removed dqm-pending labels Dec 21, 2023

cmsbuild added operations-approved fully-signed orp-approved and removed operations-pending pending-signatures orp-pending labels Dec 21, 2023

cmsbuild merged commit 693ce2c into cms-sw:master Dec 21, 2023
11 checks passed

This was referenced Dec 21, 2023

Soft muon MVA id for run3 #43622

Merged

Check that lumiSectionInterval_ is valid in StreamerOutputModuleCommon #43629

Merged

forthommel deleted the ak_update_lhc_info_direct_sim branch January 9, 2024 12:47

francescobrivio mentioned this pull request Feb 13, 2024

Remove "old" LHCInfo tag from Run3 GTs cms-AlCaDB/AlCaTools#87

Open

Glitchmin mentioned this pull request Feb 13, 2024

CTPPS update HLTPPSJetComparisonFilter to use LHCInfoCombined #43951

Merged

mmusich mentioned this pull request Feb 13, 2024

[14.0.X] update HLTPPSJetComparisonFilter to use LHCInfoCombined #43954

Merged

francescobrivio mentioned this pull request Feb 13, 2024

Update LHCInfoProducer to use LHCInfoCombined #43956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CTPPS update Run3 Direct Simulation to use old LHCInfo after LHCInfo update #42890

CTPPS update Run3 Direct Simulation to use old LHCInfo after LHCInfo update #42890

Glitchmin commented Sep 27, 2023 •

edited

Loading

cmsbuild commented Sep 27, 2023

cmsbuild commented Sep 27, 2023 •

edited

Loading

AndreaBellora Sep 28, 2023

Glitchmin Sep 28, 2023

perrotta Sep 28, 2023

makortel Sep 28, 2023

AndreaBellora Sep 28, 2023

Glitchmin Nov 14, 2023

AndreaBellora Sep 28, 2023

Glitchmin Sep 28, 2023

AndreaBellora Sep 28, 2023

AndreaBellora Sep 28, 2023

Glitchmin Sep 28, 2023 •

edited

Loading

makortel Sep 28, 2023

makortel Sep 28, 2023

AndreaBellora Sep 28, 2023

Glitchmin Nov 14, 2023

perrotta Nov 23, 2023

tvami Nov 24, 2023

perrotta Nov 27, 2023

tvami Nov 27, 2023

makortel Nov 27, 2023

cmsbuild commented Oct 31, 2023

cmsbuild commented Oct 31, 2023

smuzaffar commented Nov 6, 2023

tjavaid commented Dec 18, 2023 •

edited

Loading

civanch commented Dec 19, 2023

perrotta commented Dec 19, 2023

cmsbuild commented Dec 19, 2023

antoniovilela commented Dec 20, 2023

tjavaid commented Dec 21, 2023

antoniovilela commented Dec 21, 2023

cmsbuild commented Dec 21, 2023

		@@ -0,0 +1,3 @@
		import FWCore.ParameterSet.Config as cms

		run3_directSim = cms.Modifier()



		from Configuration.Eras.Modifier_run3_common_cff import run3_common
		run3_common.toModify(CTPPSHepMCDistributionPlotter, useNewLHCInfo = True)

CTPPS update Run3 Direct Simulation to use old LHCInfo after LHCInfo update #42890

CTPPS update Run3 Direct Simulation to use old LHCInfo after LHCInfo update #42890

Conversation

Glitchmin commented Sep 27, 2023 • edited Loading

PR description:

PR validation:

cmsbuild commented Sep 27, 2023

cmsbuild commented Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glitchmin Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Oct 31, 2023

cmsbuild commented Oct 31, 2023

smuzaffar commented Nov 6, 2023

tjavaid commented Dec 18, 2023 • edited Loading

civanch commented Dec 19, 2023

perrotta commented Dec 19, 2023

cmsbuild commented Dec 19, 2023

Comparison Summary

antoniovilela commented Dec 20, 2023

tjavaid commented Dec 21, 2023

antoniovilela commented Dec 21, 2023

cmsbuild commented Dec 21, 2023

Glitchmin commented Sep 27, 2023 •

edited

Loading

cmsbuild commented Sep 27, 2023 •

edited

Loading

Glitchmin Sep 28, 2023 •

edited

Loading

tjavaid commented Dec 18, 2023 •

edited

Loading