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 #33080

Closed
jan-kaspar opened this issue Mar 5, 2021 · 7 comments
Closed

PPS: update of era modifiers #33080

jan-kaspar opened this issue Mar 5, 2021 · 7 comments

Comments

@jan-kaspar
Copy link
Contributor

We'd like to slightly update the era modifiers for (CT)PPS. Before doing so, we'd like to know your opinion.

Let me review our needs.

  1. We need a modifier to represent all eras where PPS was active - this is to turn on PPS code in various reco and simu sequences, e.g. here
    ctpps_2016.toReplaceWith(RawToDigiTask, _ctpps_2016_RawToDigiTask)
  2. We need per-year modifiers to adjust settings of reco and simu modules, e.g. here
    ctpps_2017.toModify(ctppsProtons, apply2017Settings)

Up to now we have had ctpps_2016 used in the sense 1), but ctpps_2017 and ctpps_2018 in sense 2). This was mostly for historical reasons and we find that it is the time to rectify the situation, for the following reasons.
a) The current naming is heavily misleading and leads to mistakes, e.g. the logic combination here

(ctpps_2016 | ctpps_2017 | ctpps_2018).toModify(ctppsGeometryESModule, isRun2=True)
should have covered all PPS Run2 eras, but undesirably it covers also Run3.
b) There is no simple (and time invariant) logic combination that would represent Run2 PPS eras.

In consequence, we'd like to propose:
i) to have ctpps modifier for the case 1)
ii) to have ctpps_20XY modifiers for the case 2)
Would such a design make sense to you?

As far as we can see, such a modification would be transparent for the dominant use case when Run2_20XY are used.

We propose this change for master, no backports are considered.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2021

A new Issue was created by @jan-kaspar .

@Dr15Jones, @dpiparo, @silviodonato, @smuzaffar, @makortel, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Mar 5, 2021

I'm not sure how the proposal would resolve 2a or 2b.
Which part needs to be removed/not applied in Run3?
It sounds like you may need a pps_run3 modifier.

@makortel
Copy link
Contributor

makortel commented Mar 5, 2021

assign reconstruction, simulation, operations

(not really sure who else to add)

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 5, 2021

New categories assigned: operations,reconstruction,simulation

@mdhildreth,@slava77,@fabiocos,@franzoni,@perrotta,@qliphy,@jpata,@davidlange6,@silviodonato,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

@jan-kaspar
Copy link
Contributor Author

Now:

  • ctpps_2016 is activated for all eras where PPS contributed
  • therefore ctpps_2016 is also activated for Run3 - confusing (at least)

With the proposal:

  • ctpps_2016 would be only activated for 2016 data
  • therefore ctpps_2016 | ctpps_2017 | ctpps_2018 refers truly only for Run2, as expected

We need to remove ctpps_2016 from all but 2016 modifier chains.

@slava77
Copy link
Contributor

slava77 commented Jul 22, 2021

+reconstruction

via #33415

@jan-kaspar
Copy link
Contributor Author

This change was finally included in #33415 - closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants