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

Update of L3MuonCandidateProducerFromMuons to work also with displaced #37636

Merged

Conversation

alexsr-98
Copy link
Contributor

PR description:

This PR implements a small change in the L3MuonCandidateProducerFromMuons module. A summary of this PR can be found on this slides.

When this module was used with displaced muons a degradation in efficiency and resolution was observed. The reason was that this module was taking the innerTrack of the muon by default for momentum estimation. For displaced muons its better to take the globalTrack as shown in the attached slides (better efficiency and resolution).

We propose to add a flag to turn on the displaced reconstruction. This flag is set false by default to not interfere with the prompt reconstruction.

PR validation:

This PR passed the scram b runtests and code quality checks.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37636/29426

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @alexsr-98 for master.

It involves the following packages:

  • RecoMuon/L3MuonProducer (hlt, reconstruction)

@Martin-Grunewald, @clacaputo, @cmsbuild, @missirol, @slava77, @jpata can you please review it and eventually sign? Thanks.
@HuguesBrun, @silviodonato, @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

Comment on lines 72 to 76
if ((*muons)[i].isGlobalMuon() == 1) {
tkref = (*muons)[i].globalTrack();
} else {
tkref = (*muons)[i].muonBestTrack();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ((*muons)[i].isGlobalMuon() == 1) {
tkref = (*muons)[i].globalTrack();
} else {
tkref = (*muons)[i].muonBestTrack();
}
tkref = (*muons)[i].isGlobalMuon() ? (*muons)[i].globalTrack() : (*muons)[i].muonBestTrack();

/// produce candidates
void produce(edm::StreamID, edm::Event&, const edm::EventSetup&) const override;

private:
// L3/GLB Collection Label
edm::InputTag m_L3CollectionLabel;
edm::EDGetTokenT<reco::MuonCollection> muonToken_;
bool m_displacedReco;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool m_displacedReco;
bool const m_displacedReco;

The other two could also be made const using the following in the ctor:
(i changed muonToken_ to m_muonToken to follow the naming convention of the other data members)

    : m_L3CollectionLabel(parameterSet.getParameter<InputTag>("InputObjects")),  // standAlone Collection Label
      m_muonToken(consumes(m_L3CollectionLabel)),
      m_displacedReco(parameterSet.getParameter<bool>("DisplacedReconstruction")) {
  LogTrace(category) << " constructor called";
  produces<RecoChargedCandidateCollection>();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed the name to the one you suggest and made const the other two attributes.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37636/29431

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

Pull request #37636 was updated. @Martin-Grunewald, @clacaputo, @cmsbuild, @missirol, @slava77, @jpata can you please check and sign again.

@missirol
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2307cb/24078/summary.html
COMMIT: d109396
CMSSW: CMSSW_12_4_X_2022-04-20-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37636/24078/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-2307cb/39434.75_TTbar_14TeV+2026D88_HLT75e33+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HLT75e33+HARVESTGlobal

Summary:

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

Copy link
Contributor

@missirol missirol left a comment

Choose a reason for hiding this comment

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

Late comments from my side. Feel free to squash into 1 commit in the next update.

if (m_displacedReco) {
tkref = (*muons)[i].isGlobalMuon() ? (*muons)[i].globalTrack() : (*muons)[i].muonBestTrack();
} else {
tkref = ((*muons)[i].innerTrack().isNonnull()) ? (*muons)[i].innerTrack() : (*muons)[i].muonBestTrack();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tkref = ((*muons)[i].innerTrack().isNonnull()) ? (*muons)[i].innerTrack() : (*muons)[i].muonBestTrack();
tkref = (*muons)[i].innerTrack().isNonnull() ? (*muons)[i].innerTrack() : (*muons)[i].muonBestTrack();

@@ -1,5 +1,6 @@
import FWCore.ParameterSet.Config as cms

Copy link
Contributor

Choose a reason for hiding this comment

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

This cfi file can now be removed. I don't see it used anywhere, and it will now be created automatically via fillDescriptions.

…muons

Applied comments from PR review

Removed cfi and small change
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37636/29435

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

Pull request #37636 was updated. @Martin-Grunewald, @clacaputo, @cmsbuild, @missirol, @slava77, @jpata can you please check and sign again.

@missirol
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2307cb/24088/summary.html
COMMIT: 14733ac
CMSSW: CMSSW_12_4_X_2022-04-20-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37636/24088/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

The relvals timed out after 4 hours.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-2307cb/39434.75_TTbar_14TeV+2026D88_HLT75e33+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HLT75e33+HARVESTGlobal

Summary:

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

@missirol
Copy link
Contributor

The RelVals-INPUT error in the PR tests is unrelated to this PR, I believe.

@missirol
Copy link
Contributor

+hlt

  • adds a new option, disabled by default, to a producer used at HLT
  • no differences seen in PR tests, as expected

@clacaputo
Copy link
Contributor

+reconstruction

  • new flag in L3MuonCandidateProducerFromMuons for displaced muons
  • flag is set to false by default
  • no reco changes observed

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). 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

  • RelVals-INPUT error is unrelated

@perrotta
Copy link
Contributor

merge

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

5 participants