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.

Sign up for GitHub

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

Jump to bottom

Fixed CMS deprectation warnings in RecoMuon/TrackingTools #38310

Merged
merged 1 commit into from
Jun 16, 2022
Merged

Fixed CMS deprectation warnings in RecoMuon/TrackingTools #38310

merged 1 commit into from
Jun 16, 2022

Conversation

Dr15Jones
Copy link
Contributor

PR description:

  • changed to thread-friendly module types
  • added esConsumes calls

PR validation:

Code compiles without generating CMS deprecation warnings.

@cmsbuild cmsbuild added this to the CMSSW_12_5_X milestone Jun 9, 2022
@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38310/30477

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2022

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

It involves the following packages:

  • RecoMuon/TrackingTools (reconstruction)

@jpata, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@HuguesBrun, @abbiendi, @Fedespring, @bellan, @sscruz, @jhgoh, @CeliaFernandez, @trocino, @cericeci, @rociovilar 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

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e56de7/25399/summary.html
COMMIT: 2b3ca8f
CMSSW: CMSSW_12_5_X_2022-06-09-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38310/25399/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3652078
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3652054
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

clacaputo
clacaputo reviewed Jun 13, 2022
View reviewed changes
RecoMuon/TrackingTools/plugins/MuonErrorMatrixAdjuster.h
@@ -95,11 +95,12 @@ class MuonErrorMatrixAdjuster : public edm::EDProducer {
bool theRescale;

/// holds the error matrix parametrization
edm::ParameterSet theMatrixProvider_pset;
MuonErrorMatrix* theMatrixProvider;
std::unique_ptr<MuonErrorMatrix> theMatrixProvider;

/// hold on to the magnetic field
edm::ESHandle<MagneticField> theField;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The class MuonErrorMatrix has no const methods and therefore making it const would not work:
https://github.com/cms-sw/cmssw/blob/master/RecoMuon/TrackingTools/interface/MuonErrorMatrix.h

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, I was referring to edm::ESHandle<MagneticField> theField

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @Dr15Jones , actually I was referring to edm::ESHandle<MagneticField> theField

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ESHandle is assigned to during data processing so can't be const. In addition, when set, the ESHandle internally only allows access to the const methods of the MagneticField.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, thanks for the explanation

RecoMuon/TrackingTools/plugins/MuonErrorMatrixAdjuster.h Outdated

/// hold on to the magnetic field
edm::ESHandle<MagneticField> theField;
edm::ESGetToken<MagneticField, IdealMagneticFieldRecord> theFieldToken;
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

RecoMuon/TrackingTools/plugins/MuonErrorMatrixAdjuster.h Outdated

/// hold on to the magnetic field
edm::ESHandle<MagneticField> theField;
edm::ESGetToken<MagneticField, IdealMagneticFieldRecord> theFieldToken;
edm::ESGetToken<TrackerTopology, TrackerTopologyRcd> theHttopoToken;
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Dr15Jones
Fixed CMS deprectation warnings in RecoMuon/TrackingTools
0414bb9 
- changed to thread-friendly module types
- added esConsumes calls
@Dr15Jones Dr15Jones force-pushed the deprecatedRecoMuonTrackingTools branch from 2b3ca8f to 0414bb9 Compare June 14, 2022 15:08
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38310/30561

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

Pull request #38310 was updated. @jpata, @cmsbuild, @clacaputo, @slava77 can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e56de7/25520/summary.html
COMMIT: 0414bb9
CMSSW: CMSSW_12_5_X_2022-06-14-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38310/25520/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3659074
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3659044
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@clacaputo
Copy link
Contributor

+reconstruction

  • technical PR
  • no reco changes

@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)

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 1711d52 into cms-sw:master Jun 16, 2022
This was referenced Jun 16, 2022
Merged
Closed
Merged
@Dr15Jones Dr15Jones deleted the deprecatedRecoMuonTrackingTools branch June 28, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clacaputo clacaputo clacaputo left review comments

Assignees
No one assigned
Labels
code-checks-approved fully-signed orp-approved reconstruction-approved tests-approved
Projects
None yet
Milestone
CMSSW_12_5_X
Development

Successfully merging this pull request may close these issues.
4 participants
@Dr15Jones @cmsbuild @clacaputo @perrotta