Skip to content

HCAL: Move HF fine-grain thresholds to conditions #49012

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 2, 2025
Merged

HCAL: Move HF fine-grain thresholds to conditions #49012

merged 2 commits into from
Oct 2, 2025

Conversation

konpas17
Copy link
Contributor

@konpas17 konpas17 commented Sep 27, 2025 •
edited
Loading

PR description:

Until now the LUT generation for HF fine grain bits requires thresholds hardcoded in SimCalorimetry/HcalTrigPrimProducers/python/hcaltpdigi_cff.py. With this update, the two HF MinBias FG thresholds are retrieved from the conditions via the HcalTPParameters payload (using the previously unusedauxi1 field). A dedicated switch is also implemented to provide a fallback to the thresholds defined in the cff file.

  • For the time being, no changes are expected in the output, until the conditions are updated with non-zero values in the auxi1 field of HcalTPParameters.
  • This PR is self-contained and does not depend on other PRs or externals.
  • Additional documentation: Indico, cmshcal#288

PR validation:

  • Code compiles successfully under CMSSW_16_0_X with scram build code-checks and scram build code-format.
  • Local tests with runTheMatrix.py -l limited -i all --ibeos completed without errors.
  • Tests were performed using CaloOnlineTools/HcalOnlineDb/test/genLUT.sh. Thresholds were correctly retrieved from conditions or from the cff when auxi1 is empty or when the fallback switch is used. LUTs for HF are produced with the expected values in both cases.

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:

Not a backport. If approved, this PR will later be backported to 15_1_X, which will be used for HI this year.

@akhukhun @abdoulline @JHiltbrand

@cmsbuild cmsbuild added this to the CMSSW_16_0_X milestone Sep 27, 2025
@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 27, 2025 •
edited
Loading

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49012/46202

  • There are other open Pull requests which might conflict with changes you have proposed:
    • File SimCalorimetry/HcalTrigPrimProducers/python/hcaltpdigi_cff.py modified in PR(s): Rawp v3 #48992

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • CalibCalorimetry/HcalTPGAlgos (l1, alca)
  • CalibCalorimetry/HcalTPGEventSetup (l1, alca)
  • SimCalorimetry/HcalTrigPrimProducers (l1)

@BenjaminRS, @arunhep, @atpathak, @cmsbuild, @perrotta, @quinnanm can you please review it and eventually sign? Thanks.
@abdoulline, @bsunanda, @missirol, @mmusich, @rovere, @rsreds, @sameasy, @tocheng, @yuanchao this is something you requested to watch as well.
@ftenchini, @mandrenguyen, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@konpas17
Copy link
Contributor Author

@cmsbuild please test

@abdoulline
Copy link

"Waiting for authorized user to issue the test command."
Only authorized reviewers (listed in the first row) can do it.

@arunhep
Copy link
Contributor

arunhep commented Sep 29, 2025

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-633778/48354/summary.html
COMMIT: 09bc513
CMSSW: CMSSW_16_0_X_2025-09-29-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/49012/48354/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals-INPUT

  • 2025.0000001DAS Error

Comparison Summary

Summary:

  • You potentially added 2 lines to the logs
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3922620
  • DQMHistoTests: Total failures: 38
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3922562
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 218 log files, 188 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

perrotta
perrotta reviewed Sep 29, 2025
View reviewed changes
CalibCalorimetry/HcalTPGAlgos/src/HcaluLUTTPGCoder.cc
const HcalTPParameters* tpparameters = conditions.getHcalTPParameters();

// Read thresholds from auxi1 field
const uint32_t aux1 = tpparameters->getAuxi1();
Copy link
Contributor

Choose a reason for hiding this comment

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

getAux1() returns an int , see here: why to cast into a uint32_t in this code?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this change is going to remain, then it would be probably appropriate to modify at least the name of the method (getFGHFThresholds()?), if not the class member aux1_ itself in CondFormats/HcalObjects/interface/HcalTPParameters.h

Copy link
Contributor Author

@konpas17 konpas17 Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. So, based on the new use of auxi1 the expected input is actually a 32-bit positive integer. So that's why I actually enforce it when casting. Thus, makes sense to me to also modify the type inside the class definition if there is no objection.

@abdoulline: On the HCAL side, would this create any sort of problem?

  1. Regarding the name of the class member and method, I think it would be more appropriate to keep the aux1_ as is, considering that the remaining 16 bits of the word still remain unused and there is a plan to use them as well. That said, we can actually leave the getAux1() as is to serve the purpose of reading the whole word and create a new method, e.g. getFGHFThresholds(), that reads FG Thresholds from the first 16-bits. In this way we leave room for further utilization of the rest 16-bits while also making the code cleaner.

I can proceed with these changes if there there are no objections.

@perrotta
Copy link
Contributor

@konpas17 @cms-sw/hcal-dpg-l2

  • Can you confirm that this re-assignment of the aux1 field in the HcalTPParameters is blessed by HCAL DPG and intended to remain for its future usages?
  • Was that aux1 field ever used for other purposes in the past? I.e. couldn't it be that the new code get confused when reading from the conditions in some past GT?

@konpas17
Copy link
Contributor Author

  • Yes, these changes have been discussed and approved by the HCAL DPG experts (see the slides from the HCAL TRG-DQM I have attached).
  • To my knowledge, the aux1 field is unused, and there will be no conflict with previous non-zero values.

@abdoulline
Copy link

abdoulline commented Sep 30, 2025 •
edited
Loading

Hi all,

indeed, now this aux1 integer is supposed to be used bit-wise.
More so, there is some remaining (spare) bit space in this word, which can potentially be used in the future.

There is no previous usage of aux1, so no back-compatibility problem,
but I'd suggest to keep it as is (without modifying the base object and its getter),
we have already some experience with several "aux" member in other DB containers without a problem,
once it's appropriately (?) commented in-line in this PR (and in the config)

And even though there is no plans in the near future to re-assign this member, but

  • there were some cases in the past, when we did it
  • actual aux1 is only partly used now (2 bytes out of 4)

@perrotta
Copy link
Contributor

Thank you @abdoulline and @konpas17 for your feedbacks.
About the getter method: what about an additional one in HcalTPParameters that returns either the low or high HF FG threshold, as also suggested by @konpas17 ? (NB: this is not a request for the signature, just a suggestion: whatever update you plan for the future in that aux1 field, it could be transparently dealt with by that method, if so)

@mandrenguyen
Copy link
Contributor

Normally, 15_1_0 should be built today. Should we consider waiting a bit to accommodate the backport of this PR, such that it's part of the standard release validation?

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2025

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49012/46255

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2025

Pull request #49012 was updated. @BenjaminRS, @cmsbuild, @quinnanm can you please check and sign again.

@mandrenguyen
Copy link
Contributor

please test

@mandrenguyen
Copy link
Contributor

urgent
@cms-sw/l1-l2 please have a look

@cmsbuild cmsbuild added the urgent label Oct 1, 2025
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 1, 2025

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-633778/48409/summary.html
COMMIT: 3dc32df
CMSSW: CMSSW_16_0_X_2025-10-01-1100/el8_amd64_gcc13
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/49012/48409/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-633778/48409/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-633778/48409/git-merge-result

Comparison Summary

Summary:

@quinnanm
Copy link
Contributor

quinnanm commented Oct 2, 2025

+l1

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 2, 2025

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

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 9564701 into cms-sw:master Oct 2, 2025
10 checks passed
@konpas17 konpas17 deleted the hcal-hf_fg_thresholds branch October 3, 2025 10:42
@cmsbuild cmsbuild mentioned this pull request Oct 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@perrotta perrotta perrotta left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

alca-approved code-checks-approved fully-signed l1-approved orp-approved tests-approved urgent

Projects

None yet

Milestone

CMSSW_16_0_X

Development

Successfully merging this pull request may close these issues.

7 participants

@konpas17 @cmsbuild @abdoulline @arunhep @perrotta @mandrenguyen @quinnanm