-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 era for 2023 PbPb UPC re-reco #43378
Run3 era for 2023 PbPb UPC re-reco #43378
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43378/37860
|
A new Pull Request was created by @stahlleiton (Andre Stahl) for master. It involves the following packages:
@mandrenguyen, @rappoccio, @antoniovilela, @fabiocos, @jfernan2, @simonepigazzini, @vlimant, @davidlange6, @cmsbuild can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
from Configuration.Eras.Era_Run3_UPC_cff import Run3_UPC | ||
from Configuration.Eras.Modifier_run3_egamma_2023_cff import run3_egamma_2023 | ||
|
||
Run3_UPC_2023 = cms.ModifierChain(Run3_UPC, run3_egamma_2023) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A large fraction of Eras are named RunX_YYYY_mod
is it practical to create a bubble in the implementation of the pp era (here you re-implement the Run3
-> Run3_2023
detail). ... why do we need Run3_UPC
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed at the moment, so I will remove Run3_UPC
@@ -52,3 +52,6 @@ def thresholds( wp ) : | |||
|
|||
from Configuration.ProcessModifiers.pp_on_AA_cff import pp_on_AA | |||
pp_on_AA.toModify(lowPtGsfElectronSeeds,MinPtThreshold = 5.0) | |||
|
|||
from Configuration.Eras.Modifier_run3_upc_cff import run3_upc | |||
run3_upc.toModify(lowPtGsfElectronSeeds, ModelThresholds = thresholds("VL"), MinPtThreshold = 0.05) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't everything egamma just follow egamma_lowPt_exclusive
modifier ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved all the egamma related changes to egamma_lowPt_exclusive
@@ -92,6 +92,8 @@ | |||
originRadius = 0.2, | |||
fixedError = 4. | |||
)) | |||
from Configuration.Eras.Modifier_highBetaStar_2023_cff import highBetaStar_2023 | |||
highBetaStar_2023.toModify(pixelPairStepTrackingRegions,RegionPSet = dict(originRadius = 0.015)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a comment about the issue in the efficiency recovery; once that problem is addressed I expect that highBetaStar_2018
and highBetaStar_2023
will be identical.
It may even be more practical to use (highBetaStar_2018 & run3_upc)
here.
IIRC, the only highBeta run in 2023 was after the BPIX problem appeared, but as I recall those fills had no pixel tracker readout. @sroychow @mmusich please check and correct me if I'm wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to the approach suggested using "(highBetaStar_2018 & run3_upc)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, the only highBeta run in 2023 was after the BPIX problem appeared, but as I recall those fills had no pixel tracker readout. @sroychow @mmusich please check and correct me if I'm wrong
based on OMS this year we had the fills in the interval 9095-9168 with non-nominal beta* (120cm). All of them happened in era >= Run2023E (which is after the pixel readout incident).
As far as I can tell in none of these fills either Pixel or Strip tracker was in global DAQ excepted fills 9095 and 9128.
But in any case I don't recall the Tier-0 being moved to the highBetaStar scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once that problem is addressed I expect that highBetaStar_2018 and highBetaStar_2023 will be identical.
Incidentally (and certainly for outside of this particular PR), what's so intrinsically about 2018 of the highBetaStar_2018
modifier? Can't it just be highBetaStar
(or highBetaStar_Phase1
to signal it applies to phase-1 tracking)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
highBetaStar_Phase1
makes sense
2018 is likely the first time it made it to the configs
it would be nice to have a test workflow |
Are there instructions on how to set up a test workflow? (never done it before) |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43378/37864
|
Pull request #43378 was updated. @cmsbuild, @vlimant, @fabiocos, @davidlange6, @mandrenguyen, @simonepigazzini, @rappoccio, @jfernan2, @antoniovilela can you please check and sign again. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43378/37865
|
Pull request #43378 was updated. @vlimant, @fabiocos, @antoniovilela, @cmsbuild, @jfernan2, @rappoccio, @davidlange6, @simonepigazzini, @mandrenguyen can you please check and sign again. |
Would it be worth of opening an issue about these? |
here it is: #43488 |
Thanks for the feedback. |
@sunilUIET , @AdrianoDee , @simonepigazzini , @vlimant : Can you please sign this PR? |
@cms-sw/xpog-l2 @cms-sw/pdmv-l2 would you kindly review/sign ? |
+pdmv |
@cms-sw/xpog-l2 please review/sign this PR |
+1 |
+1 |
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. |
PR description:
This PR creates a new era "Run3_2023_UPC" meant for the 2023 PbPb UPC re-reco. It includes the following changes on top of "Run3_2023":
More details on the validations done are documented in the slide
@mandrenguyen