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 ecalMultiFitUncalibRecHit_cfi.py parameters for CC #46763

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jking79
Copy link
Contributor

@jking79 jking79 commented Nov 22, 2024

Changed ourOfTimeThresholdGain61pEB and ourOfTimeThresholdGain61mEB parameters for CC reconstruction from 3.0 to 12.0 on lines 15 and 16 of RecoLocalCalo/EcalRecProducers/python/ecalMultiFitUncalibRecHit_cfi.py

PR description:

This change address discrepancy seen in JERSF for TeV jets.

PR validation:

This PR makes an adjustment to config parameters and does not touch the code.

Changed ourOfTimeThresholdGain61pEB and ...mEB parameters for CC reconstruction from 3.0 to 12.0 on lines 15 and 16
Update ecalMultiFitUncalibRecHit_cfi.py
@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 22, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46763/42746

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @jking79 for master.

It involves the following packages:

  • RecoLocalCalo/EcalRecProducers (reconstruction)

@cmsbuild, @jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@ReyerBand, @apsallid, @argiro, @missirol, @rchatter, @thomreis, @wang0jin, @youyingli 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

@jfernan2
Copy link
Contributor

@jking79 are these constants being used in Run3 at present? Thanks

@jking79
Copy link
Contributor Author

jking79 commented Nov 22, 2024

@jfernan2 @thomreis These constants are used in the ECAL time reconstruction. The CC method, for which these changes are intended, is being considered for inclusion in 2025 running. I am pushing these changes at the instruction of ECAL DPG.

@thomreis
Copy link
Contributor

type ecal

@thomreis
Copy link
Contributor

please test

@thomreis
Copy link
Contributor

Hi @jfernan2 just to confirm that these changes have no effect on current data taking since the CC timing is not used in the default reconstruction at the moment.
With the potential re-activation of the CC timing in 2025, these configuration changes aim to give the same behaviour for reconstructed physics objects at very high energies than the currently used ratio timing. More details on this can be found here.

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ab992f/43031/summary.html
COMMIT: 5fcd1b1
CMSSW: CMSSW_15_0_X_2024-11-24-0000/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46763/43031/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 4 lines to the logs
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 46
  • DQMHistoTests: Total histograms compared: 3483722
  • DQMHistoTests: Total failures: 406
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3483296
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
  • Checked 202 log files, 172 edm output root files, 46 DQM output files
  • TriggerResults: no differences found

@jfernan2
Copy link
Contributor

assign alca

@cmsbuild
Copy link
Contributor

New categories assigned: alca

@atpathak,@consuegs,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

@jfernan2
Copy link
Contributor

FYI @malbouis @vlimant

@jfernan2
Copy link
Contributor

+1

@perrotta
Copy link
Contributor

This change address discrepancy seen in JERSF for TeV jets.

Hi @jking79 , since alca was assigned, could you please point out in the PR description some reference for that discrepancy, and possibly also some plot that shows that with the proposed new settings such a discrepancy gets resolved?

@jking79
Copy link
Contributor Author

jking79 commented Nov 26, 2024

Hello @perrotta , The discrepancy was first presented to ECALDPG here :CCtiming_checks_in_Ecal.pdf and the correction for the discrepancy was presented here : ECALDPG_20Nov2024.pdf

@perrotta
Copy link
Contributor

@jking79 , sorry for being picky, but only from the slides that you are pointing out it is far from obvious to understand:

  • what is the problem to be solved
  • how things are improved with this PR

The problem is shown at P3 in ECALDPG_20Nov2024.pdf and I assume that what you call "re-reco 2024F" in the text corresponds to what is called "HOFilter 2024F" in the figure. Then definitely 2024F re-reco has too large SFs with respect to the other distributions: I assume that this is the problem that you want to solve (even though the circle in the figure at high pt is around the other distributions, not the wrong one...). But then there is not a similar figure that shows how this PR fixes the SFs, while all other figures in the presentations looks quite obscure to non ecal experts.

For the sake of documenting in this PR what you want to fix, and how it is fixed by the PR, could you please add in the PR description:

  • A short explanation of what you want to fix
  • The figure at P3 of your presentation (possibly with some understandable label, i.e. no "HOFilter", unless it important to mention that filter).
  • The similar figure obtained after having run with this PR in both 2024C and 2024F, or alternatively (if it is hard to be derived because you have to re-reco those data) another pair of figures with an explanation that can demonstrate that with the new settings the problem gets solved.

Thank you!

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