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

Fixed sign in pat::Muon::dB() to match one obtained from the best track definition #28753

Merged
merged 7 commits into from Feb 7, 2020

Conversation

cericeci
Copy link

PR description:

The the transverse impact parameter for pat::Muon was being computed using the IPTools package. It takes the sign from the scalar product of the tangent vector to the best track at the point of closest approach to the PCA to PV vector. While this makes sense for complex objects like jets, for muon tracks it should be ~0 by definition. Thus, while the absolute dxy value makes sense the sign did not. This fixes it by switching to use the same IP sign as the dxy obtained from the best track.

This has been noted and later corrected from the nanoAOD side in
cms-nanoAOD#475

An open JIRA ticket was opened discussing the problem in the muon group
https://its.cern.ch/jira/browse/CMSMUONS-311

Expected changes

The sign obtained from pat::Muon.dB(pat::Muon::PV2D) and pat::Muon.dB(pat::Muon::BS2D) should now match what could be obtained from the track collection as pat::Muon.bestTrack()->dxy(PV or BS). Expect no changes in the absolute value itself.

PR validation

Tested locally in a couple of Z->mumu RelVal AOD samples obtaining the desired results.
No additional changes from code checks.
Minimal integration checks from runTheMatrix.py -l limited -i all passed.

cericeci added 2 commits January 16, 2020 11:55
…track instead of using the b tagging-like direction definition
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28753/13387

  • This PR adds an extra 24KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28753/13389

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @cericeci (Carlos Erice) for master.

It involves the following packages:

PhysicsTools/PatAlgos

@perrotta, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @hatakeyamak, @emilbols, @HeinerTholen, @peruzzim, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @acaudron, @jdolen, @ferencek, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @mariadalfonso, @clelange, @riga, @JyothsnaKomaragiri, @mverzett, @gpetruc, @andrzejnovak, @pvmulder this is something you requested to watch as well.
@davidlange6, @silviodonato, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@@ -1101,6 +1101,8 @@ void PATMuonProducer::embedHighLevel(pat::Muon& aMuon,
IPTools::signedTransverseImpactParameter(tt, GlobalVector(track->px(), track->py(), track->pz()), primaryVertex);
double d0_corr = result.second.value();
double d0_err = primaryVertexIsValid ? result.second.error() : -1.0;
// Now correct the sign using information from the track
d0_corr = (track->dxy(primaryVertex.position()) > 0 ? 1 : -1) * fabs(d0_corr);
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be

Suggested change
d0_corr = (track->dxy(primaryVertex.position()) > 0 ? 1 : -1) * fabs(d0_corr);
d0_corr = std::copysign(d0_corr, track->dxy(primaryVertex.position()));

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'd modify on L1102 instead. There is no significant need to assign one value and then to apply this correction.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28753/13392

  • This PR adds an extra 28KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2020

+1
Tested at: 8dd5d67
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a051dc/4486/summary.html
CMSSW: CMSSW_11_1_X_2020-02-03-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2020

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2020

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a051dc/4486/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2697068
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2696714
  • DQMHistoTests: Total skipped: 346
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 147 log files, 16 edm output root files, 34 DQM output files

@slava77
Copy link
Contributor

slava77 commented Feb 5, 2020

+1

for #28753 8dd5d67

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show differences in pat muon DB (e.g. in nanoAOD muon dxy and dxyError) or downstream in deepTau ID (which uses these 2D muon dxy and error variables)

@silviodonato
Copy link
Contributor

+1
@peruzzim @arizzi FYI

@santocch
Copy link

santocch commented Feb 7, 2020

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2020

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.

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

6 participants