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

RecHit based Ecal/Hcal muon isolation #44797

Merged
merged 4 commits into from
Apr 24, 2024
Merged

Conversation

24LopezR
Copy link
Contributor

PR description:

This PR modifies muon isolation sequences to compute muon isolation using RecHits instead of CaloTowers, since CaloTower code is not maintained anymore. It affects prompt muons, displaced muons and cosmics.
The PR was triggered by issue #43858.

PR validation:

Run over dataset: /RelValZMM_14/CMSSW_14_1_0_pre2-140X_mcRun4_realistic_v3_STD_2026D98_noPU-v6/GEN-SIM-DIGI-RAW
Isolation plots before presented weird structures, now they look more natural and close to expected.
[1] CaloTower based Ecal iso, [2] RecHit based Ecal iso
[3] CaloTower based Hcal iso, [4] RecHit based Hcal iso

[1] [2]
[3] [4]

Apart from this, standard tests with runTheMatrix WFs have been run.

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:

This PR is not a backport, and in principle it is not intended to be backported.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 22, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44797/40034

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @24LopezR for master.

It involves the following packages:

  • PhysicsTools/PatAlgos (reconstruction, xpog)
  • RecoMuon/Configuration (reconstruction)
  • RecoMuon/MuonIdentification (reconstruction)
  • RecoMuon/MuonIsolationProducers (reconstruction)

@vlimant, @jfernan2, @cmsbuild, @hqucms, @mandrenguyen can you please review it and eventually sign? Thanks.
@jdamgov, @azotz, @jhgoh, @schoef, @abbiendi, @seemasharmafnal, @gpetruc, @castaned, @jdolen, @demuller, @nhanvtran, @rappoccio, @andrea21z, @andrzejnovak, @JanFSchulte, @trocino, @gouskos, @AlexDeMoor, @mbluj, @hatakeyamak, @bellan, @gkasieczka, @mmarionncern, @ahinzmann, @HuguesBrun, @CeliaFernandez, @mariadalfonso, @Senphy, @cericeci, @Ming-Yan, @missirol, @rociovilar, @Fedespring this is something you requested to watch as well.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@mandrenguyen
Copy link
Contributor

type muon

@mandrenguyen
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bf6090/39001/summary.html
COMMIT: b6604fd
CMSSW: CMSSW_14_1_X_2024-04-22-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/44797/39001/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 12 lines from the logs
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 1111 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3319852
  • DQMHistoTests: Total failures: 8724
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3311108
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 202 log files, 165 edm output root files, 48 DQM output files
  • TriggerResults: found differences in 2 / 46 workflows

@mandrenguyen
Copy link
Contributor

+reconstruction

@hqucms
Copy link
Contributor

hqucms commented Apr 22, 2024

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

@antoniovilela
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 400c41f into cms-sw:master Apr 24, 2024
11 checks passed
@mmusich
Copy link
Contributor

mmusich commented May 10, 2024

@24LopezR @JanFSchulte

I was trying the effect of this PR on the Tracker ALCARECO samples by partially reverting my PR cms-sw/cmssw#43892 which was (in the phase-2 case) restricting the isolation cut to the tracker isolation:

diff --git a/Alignment/CommonAlignmentProducer/python/TkAlMuonSelectors_cfi.py b/Alignment/CommonAlignmentProducer/python/TkAlMuonSelectors_cfi.py
index 1999a479cb0..ae71b44233c 100644
--- a/Alignment/CommonAlignmentProducer/python/TkAlMuonSelectors_cfi.py
+++ b/Alignment/CommonAlignmentProducer/python/TkAlMuonSelectors_cfi.py
@@ -26,6 +26,6 @@ phase2_common.toModify(TkAlGoodIdMuonSelector,
                        '(abs(eta) > 2.3 & abs(eta) < 3.0 & numberOfMatches >= 0 & isTrackerMuon)'   # to recover GE0 tracks
                        )
 
-phase2_common.toModify(TkAlRelCombIsoMuonSelector,
-                       cut = '(isolationR03().sumPt)/pt < 0.1' # only tracker isolation
-                       )
+# phase2_common.toModify(TkAlRelCombIsoMuonSelector,
+#                        cut = '(isolationR03().sumPt)/pt < 0.1' # only tracker isolation
+#                        )

and then comparing it with what is in release now (by running the command below [1]), but I see a large efficiency loss in the barrel. I am wondering if isolation cut needs to be retuned after the PR.

[1]

cmsDriver.py stepALCA -s ALCA:TkAlMuonIsolated --conditions auto:phase2_realistic_T33 --datatier ALCARECO -n -1 --eventcontent ALCARECO --geometry Extended2026D110 --era Phase2C17I13M9 --fileout file:step5.root --nThreads 4 --dasquery='file dataset=/RelValWToMuNu_14TeV/CMSSW_14_1_0_pre3-140X_mcRun4_realistic_v3_STD_2026D110_noPU-v1/GEN-SIM-RECO'

@24LopezR
Copy link
Contributor Author

@mmusich Thanks for this! Let me look into it and I will reply back

@24LopezR
Copy link
Contributor Author

Hi @mmusich ,
I checked and I also observe the same inefficiency in the barrel. It comes from the huge increase in the ecal / hcal isolation values observed when changing from calo towers to rechits (see plots: at the left, before this PR; at the right, after this PR)

The effect is particularly strong for the hcal isolation. Therefore, the inefficiency isn't coming from a bad tuning of the isolation cut, and certainly a retuning of the cut will not solve it.

Seeing this, I think it was not enough to change the isolation sequences, and therefore we need also to look at the actual computation of the isolation using rechits, with the consequent deeper study. I will take care of that.

Also I missed the failed comparisons in the review of this PR. I could have noticed way before now. Sorry for that.. 😅

@mmusich
Copy link
Contributor

mmusich commented May 17, 2024

Seeing this, I think it was not enough to change the isolation sequences, and therefore we need also to look at the actual computation of the isolation using rechits, with the consequent deeper study. I will take care of that.

Hi @24LopezR thanks a lot for the follow-up and looking further into this!

Also I missed the failed comparisons in the review of this PR. I could have noticed way before now. Sorry for that.. 😅

I also somehow missed it (it might be that there's nothing that really probes this change in the existing DQM?).

@swagata87
Copy link
Contributor

hi @24LopezR
did you by any chance use all rechits within isolation cone w/o the noise cuts?

@24LopezR
Copy link
Contributor Author

Hi @swagata87 Thanks for your involvement.
I believe in L173 of RecoMuon/MuonIsolation/plugins/CaloExtractorByAssociator.cc we apply the noise cut you mention.

I'm starting to think it may be related to some tuning of the parameters used to compute the isolation (thresholds for energy, dR, noise...) Could you please point us to the EGamma code for computing this isolation please? So I can compare with what we have now. Thank you so much!

@swagata87
Copy link
Contributor

swagata87 commented May 20, 2024

Hi @24LopezR let me point you to the EGamma PRs by which they made the caloTower->recHit transition for isolation (and H/E etc), so you will see the way they modified the code+config

#33666
#40525

And, both for ECAL recHits and HCAL recHits, the DPG-recommended way to apply noise-threshold is to read the noise thresholds from global-tag. For ECAL, it is a per-crystal threshold. while for HCAL it is per-depth. For details on implementation, you may consult #43535 (ecal) and #43176 (hcal)

@srimanob
Copy link
Contributor

srimanob commented Jul 3, 2024

Hi @24LopezR @swagata87

Do I understand correctly that the remaining issue is being investigated? Thx.

@24LopezR
Copy link
Contributor Author

24LopezR commented Jul 4, 2024

Hi @srimanob
Yes, I am still investigating the issue. As an update, the suggestions by @swagata87 proved effective for fixing the ECAL isolation, but for the HCAL isolation I still observe unusually high values. I will further update when this is fixed.

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.

8 participants