-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 association cuts #27440
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27440/10747
|
A new Pull Request was created by @jan-kaspar for master. It involves the following packages: Configuration/Eras @perrotta, @cmsbuild, @franzoni, @slava77, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@@ -15,7 +15,8 @@ | |||
from Configuration.Eras.Modifier_run2_egamma_2018_cff import run2_egamma_2018 | |||
from Configuration.Eras.Modifier_run2_egamma_2017_cff import run2_egamma_2017 | |||
from Configuration.Eras.Modifier_run2_L1prefiring_cff import run2_L1prefiring | |||
from Configuration.Eras.Modifier_ctpps_2018_cff import ctpps_2018 | |||
|
|||
Run2_2018 = cms.ModifierChain(Run2_2017.copyAndExclude([run2_HEPlan1_2017, run2_muon_2017, run2_L1prefiring, run2_HLTconditions_2017, run2_egamma_2017]), |
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 you exclude ctpps_2017 here? If I understand it correctly, your idea is to have 2017 and 2018 modifiers applying on top of ("default") 2016 modifiers.
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.
Absolutely, thanks for catching this! Fixed in d8eb24c.
ctpps_2016.toModify(ctppsProtons, ApplyDefaultSettings) # applied for all Run2 years (2016, 2017 and 2018) | ||
|
||
def Apply2017Settings(ctppsProtons): | ||
ctppsProtons.association_cuts_45.xi_cut_value = 0.010 |
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.
Both these settings are identical to the "default" ones, i.e. you are not replacing anything here
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.
Right, in the POG meeting we converged on identical settings for 2016 and 2017. For symmetry (and possible future updates) I kept the code separated for 2016 and 2017.
from Configuration.Eras.Modifier_ctpps_2017_cff import ctpps_2017 | ||
from Configuration.Eras.Modifier_ctpps_2018_cff import ctpps_2018 | ||
|
||
def ApplyDefaultSettings(ctppsProtons): |
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.
Start with a lowercase letter, as for cms code rules: applyDefaultSettings
(and the same for the similar cases below)
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.
Fixed in d8eb24c.
By the way, the only difference visible in the jenkins tests is in the 2016 workflow 136.731, where an additional proton is reconstructed with the new settings. I imagine that the automated comparisons do not show differences in the 2017 or 2018 workflows just because of the low statistics. |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1
|
Please @jan-kaspar prepare the backpport to 10_6 that you have planned |
+operations the update of Configuration eras is coherent with the purpose of the PR |
+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. |
PPS: update of association cuts (backport of #27440)
This PR adjusts the "association cuts" used in multi-RP proton reconstruction. This update has been presented in RECO/AT meeting:
https://indico.cern.ch/event/830302/contributions/3477928/attachments/1871174/3079130/jan_kaspar_pps.pdf
and PPS POG meeting:
https://indico.cern.ch/event/832270/contributions/3487106/attachments/1872773/3082425/jan_kaspar_pps.pdf
The new association cuts are different for 2016, 2017 and 2018 - which is achieved by introducing new era modifiers. For backward compatibility the meaning of
ctpps_2016
is unchanged and this modifier still applies to all 2016, 17 and 18.The association cuts are modified for all 2016, 2017 and 2018.
This update is backported to the 10_6 cycle for later stages of UL re-reco in #27475.