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

Added esConsumes calls to code in RecoLocalMuon #35184

Merged
merged 14 commits into from Sep 14, 2021

Conversation

Dr15Jones
Copy link
Contributor

PR description:

  • had to propagate ConsumesCollector to many helpers/plugins
  • required some work outside of RecoLocalMuon to allow code to compile

PR validation:

Code compiles.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 7, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35184/25121

  • This PR adds an extra 152KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 7, 2021

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

  • CalibMuon/DTCalibration (alca)
  • CalibMuon/DTDigiSync (alca)
  • DQM/DTMonitorModule (dqm)
  • L1Trigger/DTTriggerPhase2 (upgrade, l1)
  • RecoLocalMuon/DTRecHit (reconstruction)
  • RecoLocalMuon/DTSegment (reconstruction)

@malbouis, @andrius-k, @yuanchao, @rekovic, @kmaeshima, @ErnestaP, @ahmad3213, @rvenditti, @cmsbuild, @AdrianoDee, @srimanob, @jfernan2, @slava77, @jpata, @francescobrivio, @cecilecaillol, @tvami can you please review it and eventually sign? Thanks.
@battibass, @abbiendi, @jhgoh, @Martin-Grunewald, @bellan, @tocheng, @mmusich this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@tvami
Copy link
Contributor

tvami commented Sep 7, 2021

@cmsbuild , please test

@tvami
Copy link
Contributor

tvami commented Sep 7, 2021

FYI @swagata87

Copy link
Contributor

@tvami tvami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove commented out code + get rid of std::cout

CalibMuon/DTCalibration/plugins/DTTPAnalyzer.cc Outdated Show resolved Hide resolved
CalibMuon/DTCalibration/plugins/DTTPAnalyzer.cc Outdated Show resolved Hide resolved
@@ -88,5 +90,6 @@ class DTTTrigCalibration : public edm::EDAnalyzer {
std::unique_ptr<DTTimeBoxFitter> theFitter;
// The module for t0 subtraction
std::unique_ptr<DTTTrigBaseSync> theSync; //FIXME: should be const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe time to make it const as the comment suggests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither std::unique_ptr<DTTTrigBaseSync const> theSync; nor const std::unique_ptr<DTTTrigBaseSync> theSync; compile. So it is unclear what that legacy comment wants.

CalibMuon/DTDigiSync/plugins/DTTTrigSyncT0Only.cc Outdated Show resolved Hide resolved
@swagata87
Copy link
Contributor

FYI @swagata87

Hi @tvami , thanks!
So I also started esConsumes migration of CalibMuon/DTCalibration as asked by Yuan from AlCa. And later I had plans to do the same for CalibMuon/DTDigiSync. These are the two packages that I'm responsible for, as DT calibration contact. Do I understand correctly that this PR already does the esConsumes migration for both these packages? If that is the case, then it's great, and there is not much to do from DT calibration side anymore. I can propagate the information to DT DPG.

@Dr15Jones
Copy link
Contributor Author

ping @cms-sw/l1-l2

@jpata
Copy link
Contributor

jpata commented Sep 13, 2021

+reconstruction

  • for c87cf4b, looking at changes in the RecoLocalMuon files
  • technical, no reco differences observed (the tests caught something in cosmics earlier which was fixed)

@srimanob
Copy link
Contributor

+Upgrade

For the upgrade part, the change is in L1Trigger/DTTriggerPhase2 which is technical change. No change is expected.

@tvami
Copy link
Contributor

tvami commented Sep 13, 2021

@rekovic @cecilecaillol @lathomas would you please sign this?

@rekovic
Copy link
Contributor

rekovic commented Sep 14, 2021

+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 now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Sep 14, 2021

+1

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.

None yet