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

Add Phase2 PbPb reco process modifier #45683

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

stahlleiton
Copy link
Contributor

@stahlleiton stahlleiton commented Aug 9, 2024

PR description:

This PR adds a new process modifier for Phase2 PbPb reconstruction. For the time being, this modifier adds the centrality producer and changes the iAbsEta threshold in calo towers used to select Phase2 HF towers in the centrality producer.

@mandrenguyen @davidlw

PR validation:

Tested with new relval 29751.85

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45683/41238

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2024

A new Pull Request was created by @stahlleiton for master.

It involves the following packages:

  • Configuration/Generator (generators)
  • Configuration/ProcessModifiers (operations)
  • Configuration/PyReleaseValidation (upgrade, pdmv)
  • Configuration/StandardSequences (operations)
  • RecoHI/HiCentralityAlgos (reconstruction)
  • RecoJets/Configuration (reconstruction)

@AdrianoDee, @antoniovilela, @bbilin, @cmsbuild, @davidlange6, @fabiocos, @jfernan2, @kskovpen, @lviliani, @mandrenguyen, @menglu21, @miquork, @mkirsano, @rappoccio, @srimanob, @subirsarkar, @sunilUIET can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @JanFSchulte, @Martin-Grunewald, @VinInn, @VourMa, @ahinzmann, @clelange, @dgulhan, @fabiocos, @felicepantaleo, @gkasieczka, @jazzitup, @jdamgov, @jdolen, @kurtejung, @makortel, @mandrenguyen, @mariadalfonso, @missirol, @mmusich, @mtosi, @nhanvtran, @rappoccio, @rovere, @sameasy, @schoef, @seemasharmafnal, @slomeo, @yenjie, @yetkinyilmaz this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@srimanob
Copy link
Contributor

Hi @stahlleiton

I am not sure I follow the PR. You add modifier, but which workflow that modifier is used? For exampe, I see new workflow 29750.0 [1] that comes with this PR, but it comes with standard pp config. Do we already have special offsets for this new modifier, i.e. see list of offsets in https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/README.md

Thx.

[1] Hydjet_Quenched_MinBias_5362GeV_2026D110_GenSimHLBeamSpot+DigiTrigger_2026D110+RecoGlobal_2026D110+HARVESTGlobal_2026D110+ALCAPhase2_2026D110

@stahlleiton
Copy link
Contributor Author

It was not clear to me how to add a relval for upgrade workflows, so that was something I wanted to ask during PR review

@srimanob
Copy link
Contributor

By the way @cms-sw/core-l2 @cms-sw/operations-l2
Could you please remind the difference between process modifiers defined in Configuration/ProcessModifiers and in Configuration/Eras? Thx.

@srimanob
Copy link
Contributor

Hi @stahlleiton

Pick the offset which is not used yet (maybe 0.85), and see example of adding workflow in https://github.com/cms-sw/cmssw/blob/master/Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py#L2007-L2031

@fabiocos
Copy link
Contributor

@srimanob Eras vs Modifiers is an old discussion... see #22718

@mandrenguyen
Copy link
Contributor

In the context of this particular PR, I'm not sure whether an era or an proc. mod. is more appropriate.
This will probably end up being like the (unfortunately named) proc. mod. pp_on_AA, which gets loaded by eras for specific years of ion data taking.
IIRC only eras can be loaded by the T0 configurations, but this consideration is not going to be relevant for some time still.
If no one objects, I think we can just go with the proposal in the PR to modify the Phase-2 heavy-ion reco with a proc. mod.

@srimanob
Copy link
Contributor

Hi @fabiocos @mandrenguyen
Thanks for comment. To be clear, I am not talking about era, I just wondering what is different between 2 directories of modifier, e.g.
Configuration/Eras/python/Modifier_run2_muon_2018_cff.py
and
Configuration/ProcessModifiers/python/vectorHits_cff.py

Both have the same structure,

import FWCore.ParameterSet.Config as cms
something =  cms.Modifier()

@makortel
Copy link
Contributor

I just wondering what is different between 2 directories of modifier

The intended difference was something around

  • Configuration/Eras for modifiers related to data taking periods
  • Configuration/ProcessModifiers for modifiers that are more or less independent of data taking periods

(I think I only rephrased what @fabiocos had written in #22718 (comment))

My feeling is there is some gray area between the two, and I'd bet we have some modifiers that could be argued to be misplaced.

@srimanob
Copy link
Contributor

Thanks @makortel

I think for this case, it may fit well under Configuration/Eras as this will be called for Phase-2 reco. However, no objection to go as proposed. It seems grey to me.

I think if we have the new workflow offset, we should be ready to go.

@mandrenguyen
Copy link
Contributor

Thanks @makortel

I think for this case, it may fit well under Configuration/Eras as this will be called for Phase-2 reco. However, no objection to go as proposed. It seems grey to me.

I think if we have the new workflow offset, we should be ready to go.

I'm not sure that a modifier that pertains to the Phase-2 detector qualifies as being appropriate for a specific run period.
The current pp_on_AA proc. mod. is implicitly only appropriate for the Phase-1 detector. It is called by specific eras for each running period. I would propose to continue with this model.

Probably a cleaner approach would be to have an overall heavy-ion modifier for stuff that's common to both the Phase-1 and Phase-2 detectors (e.g., storing the centrality object). Then we could have additional heavy-ion modifiers for Phase-1 and Phase-2, probably still as proc. mods.
Then the individual eras for specific heavy-ion data taking periods will simply load these modifier chains.
This could be done in a future PR.

@stahlleiton
Copy link
Contributor Author

Added a new Phase-2 PbPb workflow. The relval number is: 29751.85

@mandrenguyen
Copy link
Contributor

please test workflow 29751.85

@srimanob
Copy link
Contributor

+Upgrade

Re-sign.

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 32KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-45bdef/40997/summary.html
COMMIT: d4adc0b
CMSSW: CMSSW_14_1_X_2024-08-17-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45683/40997/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-45bdef/40997/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-45bdef/40997/git-merge-result

Comparison Summary

Summary:

@mandrenguyen
Copy link
Contributor

+reconstruction

@AdrianoDee
Copy link
Contributor

+pdmv

@menglu21
Copy link
Contributor

menglu21 commented Aug 19, 2024

+1

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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 cmsbuild merged commit af34144 into cms-sw:master Aug 19, 2024
11 checks passed
@stahlleiton stahlleiton deleted the Phase2_PbPb_CMSSW_14_1_X branch August 19, 2024 11:04
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.

8 participants