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

Muon Ecal rechit isolation thresh from GT, revert Hcal/H0 iso to calotower #45613

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

24LopezR
Copy link
Contributor

@24LopezR 24LopezR commented Aug 1, 2024

PR description:

This PR is a follow up of PR #44797, concerning muon ecal/hcal/h0 isolation.

  • Ecal rechit based isolation is fixed by applying noise thresholds from GT
  • Hcal/H0 isolation is temporally reverted to calotower based (as before PR RecHit based Ecal/Hcal muon isolation #44797) until the rechit based computation is fully understood and solved.

As well, this PR aims to solve issue #43858. The variable Muon::isolationR03().emEt should no longer be the source of the issue observed, although hcal isolation can still be a cause of the inefficiency when applying the global isolation cut. A check for this is needed.

Note: HLT files are slightly modified due to the introduction of new parameters, but hltmuons are not modified.

PR validation:

Ecal/Hcal isolation is compared w.r.t CMSSW_14_1_0_pre2 (before PR #44797).
~1500 events of /RelValZMM_14/CMSSW_14_1_0_pre2-140X_mcRun4_realistic_v3_STD_2026D98_noPU-v6/GEN-SIM-DIGI-RAW have been run (no PU sample, so muon isolation should be close to 0).
Weird structures in Ecal iso are fixed, while Hcal iso remains untouched.

Left: CMSSW_14_1_0_pre2 (reference)
Right: this PR (target)


Also, basic battery of test workflows has been run successfully.

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 Aug 1, 2024

cms-bot internal usage

@24LopezR
Copy link
Contributor Author

24LopezR commented Aug 1, 2024

@mmusich FYI

@mmusich
Copy link
Contributor

mmusich commented Aug 1, 2024

-hlt

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45613/41113

  • Found files with invalid states:
    • RecoMuon/MuonIsolationProducers/python/muIsoDeposits_cff.py.1muIsoDeposits_cff.py:

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2024

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

It involves the following packages:

  • HLTrigger/Configuration (hlt)
  • RecoMuon/MuonIdentification (reconstruction)
  • RecoMuon/MuonIsolation (reconstruction)
  • RecoMuon/MuonIsolationProducers (reconstruction)

@cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@CeliaFernandez, @Fedespring, @HuguesBrun, @JanFSchulte, @Martin-Grunewald, @SohamBhattacharya, @VourMa, @abbiendi, @andrea21z, @bellan, @cericeci, @jhgoh, @missirol, @mmusich, @rociovilar, @rovere, @silviodonato, @trocino this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented Aug 1, 2024

@24LopezR doesn't CaloExtractorByAssociator have a fillDescriptions method?
If yes please provide one, such that the HLT can use the defaults from it (without modifying directly the menu). Only TSG/STORM can / will manipulate those files as the configuration needs to be changed in confDB as well!
In case it's not possible please write a customization function to provide defaults in HLTrigger/Configuration/python/customizeHLTforCMSSW.py

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45613/41120

@24LopezR
Copy link
Contributor Author

24LopezR commented Aug 1, 2024

@mmusich I added the default values for the necessary parameters in the MuonIdProducer. Tested again and it seems to work fine.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2024

Pull request #45613 was updated. @cmsbuild, @jfernan2, @mandrenguyen can you please check and sign again.

@mmusich
Copy link
Contributor

mmusich commented Aug 1, 2024

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2024

+1

Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cf138a/40723/summary.html
COMMIT: 46ae6f2
CMSSW: CMSSW_14_1_X_2024-08-01-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45613/40723/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
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 1332 differences found in the comparisons
  • DQMHistoTests: Total files compared: 45
  • DQMHistoTests: Total histograms compared: 3423977
  • DQMHistoTests: Total failures: 11315
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3412642
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 44 files compared)
  • Checked 196 log files, 165 edm output root files, 45 DQM output files
  • TriggerResults: found differences in 2 / 43 workflows

@mandrenguyen
Copy link
Contributor

type muon

@cmsbuild cmsbuild added the muon label Aug 1, 2024
@mandrenguyen
Copy link
Contributor

+1
Comparisons show changes only to muon isolation, as expected.
A bit unfortunate that this prolongs the use of caloTowers. Hopefully that can be sorted in the near future.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 1, 2024

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.

@cmsbuild cmsbuild merged commit 6d4162d into cms-sw:master Aug 1, 2024
11 checks passed
@@ -1532,6 +1532,13 @@ void MuonIdProducer::fillDescriptions(edm::ConfigurationDescriptions& descriptio
edm::ParameterSetDescription descCalo;
descCalo.setAllowAnything();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line setAllowAnything still needed, given you added the relevant parameters with values below?

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 added below only the new parameters introduced in this PR, but there are many more. I do not know if it is safe to remove the setAllowAnything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah OK - you'd have to add all of them ...

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.

5 participants