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

Change from numberOfMatches() to segment number in DisplacedMuonFilterProducer #41496

Merged

Conversation

24LopezR
Copy link
Contributor

@24LopezR 24LopezR commented May 3, 2023

PR description:

This PR intends to fix an unexpected behavior observed when validating the performance of the slimmedDisplacedMuons collection in MiniAOD samples.
The issue consists in a bad filtering of highly displaced muons caused by the definition of muon.numberOfMatches(), which was proved to not be appropriate to effectively select displaced muons when no tracker track is reconstructed.
The fix in this PR proposes to change this variable for another one. Now, the segments of the track are counted through the reco::Track object itself, which yields a more accurate description of the standalone muon track properties.
This issue was reported to and discussed with the MUO POG, a detailed description can be found at:
https://indico.cern.ch/event/1263952/ ("Update on displaced muon collection performance studies").

Muon number expected to increase in muons from the slimmedDisplacedMuons collection which are not .isGlobal() nor .isTracker() in high displaced signals e.g DisplacedSUSY and Cosmics. These muons are not expected to change in prompt signals except for some small fluctuations. Standard muons should not be affected in any way.

The fix has been checked to work properly in cosmics data and H->SS MC signal (higher and lower displacement).

See attached plot for a comparison of muon $d_{xy}$ distribution for a highly displaced H->SS signal (ctau=4000mm) between the old filter (red) and the fix in this PR (blue).
HTo2LongLived_400_150_4000_h_dxy

Other authors: @CeliaFernandez

PR validation:

The code changes have passed all standard runTheMatrix test workflows.
No significant increase in size of MiniAOD is expected (tested on ttbar workflow 11834.21)

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:

We would like to have this also backported to 13_0_0.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41496/35368

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2023

A new Pull Request was created by @24LopezR (Ruben Lopez) for master.

It involves the following packages:

  • PhysicsTools/PatAlgos (xpog, reconstruction)

@swertz, @vlimant, @clacaputo, @cmsbuild, @simonepigazzini, @mandrenguyen can you please review it and eventually sign? Thanks.
@AlexDeMoor, @rappoccio, @gouskos, @jdolen, @JyothsnaKomaragiri, @ahinzmann, @AnnikaStein, @schoef, @emilbols, @jdamgov, @mbluj, @nhanvtran, @gkasieczka, @hatakeyamak, @gpetruc, @azotz, @mariadalfonso, @demuller, @andrzejnovak, @seemasharmafnal, @mmarionncern this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@mandrenguyen
Copy link
Contributor

please test

@mandrenguyen
Copy link
Contributor

type muon

@cmsbuild
Copy link
Contributor

cmsbuild commented May 3, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3b626c/32333/summary.html
COMMIT: db1ce80
CMSSW: CMSSW_13_1_X_2023-05-02-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41496/32333/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 3 lines from the logs
  • Reco comparison results: 466 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3460877
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3460853
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@CeliaFernandez
Copy link
Contributor

I had a look at the failed reco comparison results. I would say these are consistent with the changes expected in prompt samples:

Affected muons are slimmedDisplacedMuons with are only displacedStandAloneMuons (StandAloneMuon type in the collection i.e. 00001000). These kind of muons would be the only ones for which the modified lines are executed. See for example in 10224.0 comparison:

MuonTypeAffected

The changes in the distributions arise because the number of muons is changing i.e. different muons are filtered w.r.t. reference. The proposed changes should either:

  • Increase the number of SA-only displaced muons with numberOfMatches < 2: because they pass the new filter but failed the old one.
  • Decrease the number of SA-only displaced muons with numberOfMatches >= 2: because they passed the old filter but now they fail the new one.

This effect can be also checked in workflow 136.793:

136p793_c_patMuons_slimmedDisplacedMuons__reRECO_obj_numberOfMatches

Copy link
Contributor

@mandrenguyen mandrenguyen left a comment

Choose a reason for hiding this comment

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

If you moved the minPtSTA check earlier in the code, would it save you from having to calculate the number of segments when the pT threshold condition isn't met?

@24LopezR
Copy link
Contributor Author

24LopezR commented May 9, 2023

I moved the minPtSTA check to avoid computing nsegments unnecessarily when the condition is not met.

I also checked that the behaviour of the filter remains the same. See attached plot of DSA muon $|d_{xy}|$ variable (both histograms overlap):

image

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41496/35485

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2023

Pull request #41496 was updated. @swertz, @vlimant, @clacaputo, @cmsbuild, @simonepigazzini, @mandrenguyen can you please check and sign again.

@mandrenguyen
Copy link
Contributor

please test

@mandrenguyen
Copy link
Contributor

please test
wake up, bot!

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3b626c/32505/summary.html
COMMIT: e1e435a
CMSSW: CMSSW_13_2_X_2023-05-08-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41496/32505/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 2 lines from the logs
  • Reco comparison results: 471 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3460836
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3460808
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@mandrenguyen
Copy link
Contributor

+reconstruction
Changes consistent with PR description

@simonepigazzini
Copy link
Contributor

+xpog

@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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
Copy link
Contributor

+1

While this is perfectly fine for 13_2, we need to discuss the backport to 13_0 since this changes prompt workflows already in production.

@simonepigazzini
Copy link
Contributor

Since prompt is over can we backport this to 13_0? It looks like some EXO analysis could benefit from this fix to the displaced muons collection if this goes into the reMINI campaign for 2022 data/MC.

@rappoccio @antoniovilela @cms-sw/ppd-l2

@24LopezR
Copy link
Contributor Author

@simonepigazzini Yes, it can be backported to 13_0 (I see there is already a PR for this). Indeed, it would be very benefitial for EXO analyses.

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.

7 participants