Skip to content

Conversation

@igv4321
Copy link
Contributor

@igv4321 igv4321 commented Sep 12, 2024

PR description:

Adding infrastructure for modeling Hcal front-end pulse shapes channel-by-channel. The pulse shapes were determined by phase scans, as described in https://indico.cern.ch/event/1336438/contributions/5626382/attachments/2733633/4753627/HBHE_Pulse_shape_study.pdf. The new pulse shapes have not yet been turned on, pending final timing adjustment.

PR validation:

The usual matrix text were run. Also, privately, the new pulse shapes were switched on (parameter "useChannelShapes" in HBHEPhase1Reconstructor_cfi.py was set True), and the code appears to produce reasonable results on the 2023 data workflow, as indicated by the subsequent Hcal rechit dump.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 12, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45995/41766

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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45995/41767

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • CalibCalorimetry/HcalAlgos (alca)
  • DataFormats/HcalRecHit (reconstruction)
  • RecoLocalCalo/Configuration (reconstruction)
  • RecoLocalCalo/HcalRecAlgos (reconstruction)
  • RecoLocalCalo/HcalRecProducers (reconstruction)

@cmsbuild, @consuegs, @jfernan2, @mandrenguyen, @perrotta, @saumyaphor4252 can you please review it and eventually sign? Thanks.
@ReyerBand, @abdoulline, @apsallid, @argiro, @bsunanda, @mariadalfonso, @missirol, @mmusich, @rchatter, @rovere, @rsreds, @sameasy, @thomreis, @tocheng, @wang0jin, @youyingli, @yuanchao 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

please test

# indicating whether we are using it
channelShapesLabel = cms.string("HcalDataShapes"),
useChannelShapes = cms.bool(False),

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, I'm not a reviewer, I just have a general question on these developments.

What is their target (if there is one) for going in production ? 2025 data-taking ?

Are these changes only needed for the HCAL offline reconstruction, or are they also supposed to be used at HLT ?

Ever since the heterogeneous version of the HCAL local reconstruction was ported to Alpaka (#44910), the plugin HBHEPhase1Reconstructor is not used anymore in the Run-3 HLT menus (in other words, HLT is blind to this PR, as far as I understand).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be useful for 2023 and 2024 re-reco as well as for the future data taking. Unfortunately, at the moment we don't have an alpaka expert in the HCAL DPG, so there is no concrete plan how to get this to work in the HLT. Of course, help from Martin Kwok (or any other alpaka expert) would be appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification.

Notifying @kakwok, @fwyzard and @cms-sw/hlt-l2.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have to say that this

for the future data taking

combined with

there is no concrete plan how to get this to work in the HLT.

sounds to me like the HCAL DPG is not taking full ownership of the HCAL code running in production today in the HLT. I don't know the quantitative impact of these channel-dependent pulse shapes, but I would assume that we (well, HCAL specifically) should make an effort to keep HLT and offline as close as possible to each other.

@cmsbuild
Copy link
Contributor

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0a1618/41506/summary.html
COMMIT: 3c0b14f
CMSSW: CMSSW_14_2_X_2024-09-12-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45995/41506/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
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 44
  • DQMHistoTests: Total histograms compared: 3331158
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3331135
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
  • Checked 193 log files, 163 edm output root files, 44 DQM output files
  • TriggerResults: no differences found

label = cms.string('default'),
t0 = cms.uint32(0),
pulse = cms.vdouble(
5.22174e-12, 7.04852e-10, 3.49584e-08, 7.78029e-07, 9.11847e-06,
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this large amount of parameters be better stored in the database, or at least in some external cms-data repository? This makes this file ~ unreadable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was discussed with the Hcal calibration group. However, this is the first time we are introducing this channel-by-channel dependence since the beginning of CMS operations, and the pulse shapes are not expected to change more often than once per era, so it was decided to keep the information in a config file. While this file is large (and, of course, computer-generated), its structure is rather simple, as can be seen from the "fillDescriptions" function of the HcalPulseShapesEP.cc code.

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach is not acceptable.

Please move the pulse shapes either to a text file in an external cms-data package, or to a database payload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please substantiate your statement? Haven't seen anything against this approach in CMSSW guidelines and can't understand why it can be harmful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you seen many other modules with a configuration that takes twelve thousand lines of python code ?

Co-authored-by: Andrea Bocci <fwyzard@gmail.com>
@abdoulline
Copy link

abdoulline commented Sep 16, 2024

For a support, you can find the names of the AlCaDB contacts in HCAL from this list: https://twiki.cern.ch/twiki/bin/viewauth/CMS/AlCaDBContacts

I'm afraid HCAL (three) names are obsolete. Yildiray Komurcu to replace them.

@cmsbuild
Copy link
Contributor

Milestone for this pull request has been moved to CMSSW_15_0_X. Please open a backport if it should also go in to CMSSW_14_2_X.

@cmsbuild cmsbuild modified the milestones: CMSSW_14_2_X, CMSSW_15_0_X Nov 22, 2024
@fwyzard
Copy link
Contributor

fwyzard commented Dec 10, 2024

type hcal

@cmsbuild cmsbuild added the hcal label Dec 10, 2024
@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 7, 2025

Milestone for this pull request has been moved to CMSSW_15_1_X. Please open a backport if it should also go in to CMSSW_15_0_X.

@cmsbuild
Copy link
Contributor

Milestone for this pull request has been moved to CMSSW_16_0_X. Please open a backport if it should also go in to CMSSW_15_1_X.

@igv4321
Copy link
Contributor Author

igv4321 commented Dec 5, 2025

Superseeded by #49556

@igv4321 igv4321 closed this Dec 5, 2025
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