Modification to move ZDC trigger spacing to TP channel parameters conditions. #46537

hjbossi · 2024-10-28T22:40:21Z

PR description:

This PR takes a parameter hard-coded in the HCAL LUT generation for the ZDC and replaces it with a second auxiliary parameter from the HCAL TP Channel Parameters conditions. This is in order to change the granularity of the ZDC trigger decision (currently set to 1 count = 50 GeV) in coordination with ZDC gain changes set forth by changing the operating voltage. Whenever the conditions are updated to modify this parameter, this will also require a modification of the L1 menu. Therefore this PR will need to be carefully synchronized for deployment.

PR validation:

This PR was tested locally on 2024 HI data taken with spurious collisions over the weekend.

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 intended for HI data-taking and will require backport to CMSSW_14_1_X.

Tagging @mandrenguyen , @JHiltbrand, and @Michael-Krohn.

cmsbuild · 2024-10-28T22:40:46Z

cms-bot internal usage

cmsbuild · 2024-10-28T22:42:39Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46537/42399

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

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46537/42399/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46537/42399/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

JHiltbrand · 2024-10-29T06:37:18Z

Hi @hjbossi ,

I assume you have a new HcalTPChannelParameters.txt in-hand with appropriate, non-zero values filled in for auxi2, ready to pass to our HCAL conditions expert ?

ATTN: @abdoulline @yildirayk

hjbossi · 2024-10-29T08:17:47Z

Hi @hjbossi ,

I assume you have a new HcalTPChannelParameters.txt in-hand with appropriate, non-zero values filled in for auxi2, ready to pass to our HCAL conditions expert ?

ATTN: @abdoulline @yildirayk

Yes, I do, of course for the ZDC channels, which would need to be merged with the HCAL channels. We actually also use auxi1 for the "weight" corresponding to the fraction of OOTPU (see here), so that is non-zero as well.

cmsbuild · 2024-10-29T09:35:14Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46537/42401

cmsbuild · 2024-10-29T09:35:38Z

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

It involves the following packages:

CalibCalorimetry/HcalTPGAlgos (l1, alca)

@aloeliger, @atpathak, @cmsbuild, @consuegs, @epalencia, @perrotta can you please review it and eventually sign? Thanks.
@abdoulline, @bsunanda, @mmusich, @rsreds, @tocheng, @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

mandrenguyen · 2024-10-29T10:31:24Z

please test workflow 142

perrotta

These comments are not fundamental: code will be cleaner if implemented, but you can skip if you are in a hurry to merge them

perrotta · 2024-10-29T12:14:45Z

CalibCalorimetry/HcalTPGAlgos/src/HcaluLUTTPGCoder.cc

      int weight = tpParam->getauxi1();
+      int factorGeVPerCount = tpParam->getauxi2();


Suggested change

int weight = tpParam->getauxi1();

int factorGeVPerCount = tpParam->getauxi2();

const int weight = tpParam->getauxi1();

const int factorGeVPerCount = tpParam->getauxi2();

perrotta · 2024-10-29T12:15:14Z

CalibCalorimetry/HcalTPGAlgos/src/HcaluLUTTPGCoder.cc

      int lutId = getLUTId(cell);
      int lutId_ootpu = lutId + sizeZDC_;


Suggested change

int lutId = getLUTId(cell);

int lutId_ootpu = lutId + sizeZDC_;

const int lutId = getLUTId(cell);

const int lutId_ootpu = lutId + sizeZDC_;

took suggestion, resolved in latest version

perrotta · 2024-10-29T12:22:14Z

CalibCalorimetry/HcalTPGAlgos/src/HcaluLUTTPGCoder.cc

+            lut[adc] = std::min(std::max(0, int((adc2fC(adc) - ped) * gain * rcalib / factorGeVPerCount)), MASK);
+            lut_ootpu[adc] = std::min(
+                std::max(0, int((adc2fC(adc) - ped) * gain * rcalib * weight / (factorGeVPerCount * 256))), MASK);


Suggested change

lut[adc] = std::min(std::max(0, int((adc2fC(adc) - ped) * gain * rcalib / factorGeVPerCount)), MASK);

lut_ootpu[adc] = std::min(

std::max(0, int((adc2fC(adc) - ped) * gain * rcalib * weight / (factorGeVPerCount * 256))), MASK);

auto lut_term = (adc2fC(adc) - ped) * gain * rcalib / factorGeVPerCount;

lut[adc] = std::clamp(int(lut_term), 0, MASK);

lut_ootpu[adc] = std::clamp(int(lut_term * weight / 256), 0, MASK);

(you must #include <algorithm> if you do so)

Took suggestion, resolved in latest version

hjbossi · 2024-10-29T14:54:27Z

These comments are not fundamental: code will be cleaner if implemented, but you can skip if you are in a hurry to merge them

Thank you for your comments @perrotta ! We have implemented these in the latest version. Please let us know of any further comments!

perrotta · 2024-10-31T12:52:08Z

2. Yes, it is expected that this message prints once per event as the creation of the LUT is associated with beginRun. I have also checked locally that this prints once per event .

Then it will be printed once per run, not once per event. Which could still be quite a lot for large productions which spans over several runs (but perhaps large productions are only setup to print Errors, not Warnings, and therefore it could be still affordable).
Let discuss it later on.
In the meanwhile I launch the tests, so that I can sign if we agree that the current implementation is correct.

perrotta · 2024-10-31T12:53:06Z

please test workflow 142

cmsbuild · 2024-10-31T18:31:59Z

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-535e76/42513/summary.html
COMMIT: 2626353
CMSSW: CMSSW_14_2_X_2024-10-30-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46537/42513/install.sh to create a dev area with all the needed externals and cmssw changes.

DAS Queries: The DAS query tests failed, see the summary page for details.

RelVals-INPUT

2024.0000012024.000001_RunJetMET02024D_10k/step1_dasquery.log
2024.0010012024.001001_RunZeroBias2024D_10k/step1_dasquery.log
2024.1010012024.101001_RunZeroBias2024C_10k/step1_dasquery.log

Expand to see more relval errors ...

2024.100001
2024.000001
2024.001001
2024.100001
2024.101001

Comparison Summary

Summary:

You potentially added 131 lines to the logs
Reco comparison results: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 47
DQMHistoTests: Total histograms compared: 3632136
DQMHistoTests: Total failures: 454
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3631662
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 46 files compared)
Checked 205 log files, 174 edm output root files, 47 DQM output files
TriggerResults: no differences found

perrotta · 2024-10-31T18:49:21Z

+alca

The DAS errors in a few workflows are unrelated
The code will release a few lines of LogWarning message per run, until the conditions are updated with the missing content: this will be fixed at the next condition updates, and in any case LogWarning shouldn't get triggered in production-like workflows (correct?)

mandrenguyen · 2024-10-31T19:25:18Z

ignore tests-rejected with ib-failure

mandrenguyen · 2024-10-31T19:26:08Z

+1

aloeliger · 2024-11-01T06:57:50Z

+l1

cmsbuild · 2024-11-01T06:58:13Z

This pull request is fully signed and it will be integrated in one of the next master IBs (test failures were overridden). This pull request will be automatically merged.

mmusich · 2024-11-15T21:08:53Z

CalibCalorimetry/HcalTPGAlgos/src/HcaluLUTTPGCoder.cc

+      const int weight = tpParam->getauxi1();
+      int factorGeVPerCount = tpParam->getauxi2();
+      if (factorGeVPerCount == 0) {
+        edm::LogWarning("HcaluLUTTPGCoder")


since after this PR was merged, logs of Phase 2 step2 (DIGI:pdigi_valid,L1TrackTrigger,L1,L1P2GT,DIGI2RAW,HLT:@relvalRun4) workflows are full of:

%MSG-w HcaluLUTTPGCoder: HcalTPGCoderULUT:HcalTPGCoderULUT@callESModule 15-Nov-2024 15:59:20 CET Run: 1 Event: 701 WARNING: ZDC trigger spacing factor, taken from auxi2 field of HCALTPChannelParameters for the cell with (zside, section, channel) = (1 , 1 , 1) is set to the default value of 0, which is an incompatible value for a spacing factor. Setting the value to 50 and continuing. %MSG %MSG-w HcaluLUTTPGCoder: HcalTPGCoderULUT:HcalTPGCoderULUT@callESModule 15-Nov-2024 15:59:20 CET Run: 1 Event: 701 WARNING: ZDC trigger spacing factor, taken from auxi2 field of HCALTPChannelParameters for the cell with (zside, section, channel) = (1 , 1 , 2) is set to the default value of 0, which is an incompatible value for a spacing factor. Setting the value to 50 and continuing. %MSG %MSG-w HcaluLUTTPGCoder: HcalTPGCoderULUT:HcalTPGCoderULUT@callESModule 15-Nov-2024 15:59:20 CET Run: 1 Event: 701 WARNING: ZDC trigger spacing factor, taken from auxi2 field of HCALTPChannelParameters for the cell with (zside, section, channel) = (1 , 1 , 3) is set to the default value of 0, which is an incompatible value for a spacing factor. Setting the value to 50 and continuing. %MSG %MSG-w HcaluLUTTPGCoder: HcalTPGCoderULUT:HcalTPGCoderULUT@callESModule 15-Nov-2024 15:59:20 CET Run: 1 Event: 701 WARNING: ZDC trigger spacing factor, taken from auxi2 field of HCALTPChannelParameters for the cell with (zside, section, channel) = (1 , 1 , 4) is set to the default value of 0, which is an incompatible value for a spacing factor. Setting the value to 50 and continuing. %MSG

see, e.g. step2 of 29634.0.

@perrotta @hjbossi in the PR signature there's written:

The code will release a few lines of LogWarning message per run, until the conditions are updated with the missing content: this will be fixed at the next condition updates, and in any case LogWarning shouldn't get triggered in production-like workflows (correct?)

are we expecting this to be fixed in Phase 2 conditions as well? when is this going to happen?

Thank you @mmusich for reporting. These logs are printed once per run: they are certainly annoying, but probably still affordable. In any case, certainly also the HCALTPChannelParameter tag in the Phase2 GTs should be eventually updated to take into account the second parameter searched for in those conditions.
However, my point here would be different: why ZDC is enabled at all in Phase2 workflows? Maybe the recently merged PR #46570 from @bsunanda testifies that HCAL/ZDC people is aware of the issue and they are going to provide a mechanism to solve it?

@bsunanda do you mind letting us know the status on this? I am also not sure why ZDC is enabled in Phase 2 workflows at the moment.

these logs are printed once per run: they are certainly annoying, but probably still affordable.

in the log of e.g. step2 of 29634.0 I count these logs 13 times and not one.
In any case, I am not sure what "affordable" means in this context. If all PRs were processed by the standards of this PR, the logs of production workflows will quickly be swamped by a large amount of warnings and errors, rendering them not very useful for the whole collaboration (as when the log file is full of warnings it's difficult to spot additional, perhaps more serious, problems).
I understand that this PR might have been needed for operations, but what bothers me here, is that apparently there is no clear roadmap from the experts about the clean-up (and the PR was merged 2 weeks ago).

In the log of e.g. step2 of 29634.0, as in all other logs affected, there are warnings for the 13 ZDC channels printed once per run, as I wrote. Having the warnings printed only once per run was something that we wanted to get assured during the review, in order not to flood the outputs with too much output even during this intermediate period before all relevant GTs could get updated.

This is certainly annoying, as I wrote, and it must be fixed (thank you again for having pointed it out for the Phase2 case, where it was indeed overlooked because I did not think that ZDC would have been digitized in Phase2 workflows). For "affordable" I mean that having those warnings listed only once at the begin of run should have probably still allowed to spot other warning messages, certainly in the other events of the runs, but quite likely also at that begin of run.

As examples of much less affordable warning reports let me point out (from some more or less randomly chosen workflows) the continuously repeated messages from ElectronSeedProducer:ecalDrivenElectronSeeds in step3 of wf 136.874, or the ones from TrackRefitter or PrimaryVertexAnalyzer4PUSlimmed (and probably also a few more) from step3 of wf 181.1, which are there from long time and apparently nobody cares about.

abdoulline · 2024-11-16T09:46:47Z

Hi all,
we're updating MC tag of HcalTPChannelParameters to silence the warnings in question in those wfs, where legacy ZDC SIM/DIGI are present.

For Phase2 - I've suggested to @bsunanda to make ZDC Digi switching off not an option, but just a permanent modification...

yildirayk · 2024-11-16T11:33:25Z

HcalTPChannelParameters_2024_v2.0_mc has been created by replacing the auxi2 values of ZDC channels by 50 and uploaded to the database and queued to the 142X_mcRun3_2025* tags.

cmsbuild added this to the CMSSW_14_2_X milestone Oct 28, 2024

cmsbuild added l1-pending alca-pending pending-signatures tests-pending orp-pending code-checks-pending labels Oct 28, 2024

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 28, 2024

hjbossi force-pushed the PRforMaster_HCALLUTScaling branch from 4ea6a5f to 476bdb1 Compare October 29, 2024 09:33

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 29, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 29, 2024

cmsbuild added tests-started and removed tests-pending labels Oct 29, 2024

perrotta reviewed Oct 29, 2024

View reviewed changes

hjbossi force-pushed the PRforMaster_HCALLUTScaling branch from 476bdb1 to 3d31f37 Compare October 29, 2024 14:53

cmsbuild added tests-pending code-checks-pending and removed tests-started code-checks-approved labels Oct 29, 2024

cmsbuild added tests-started and removed tests-pending labels Oct 31, 2024

cmsbuild added tests-rejected and removed tests-started labels Oct 31, 2024

cmsbuild added alca-approved and removed alca-pending labels Oct 31, 2024

cmsbuild added tests-approved tests-ib-failure and removed tests-rejected labels Oct 31, 2024

cmsbuild added orp-approved and removed orp-pending labels Oct 31, 2024

cmsbuild added l1-approved fully-signed and removed l1-pending pending-signatures labels Nov 1, 2024

cmsbuild merged commit 7d3d80e into cms-sw:master Nov 1, 2024
10 of 11 checks passed

cmsbuild mentioned this pull request Nov 1, 2024

Add GloParT inference facility #46523

Merged

mmusich reviewed Nov 15, 2024

View reviewed changes

perrotta mentioned this pull request Nov 16, 2024

Run3hcx378A Provide a way to avoid Digitization for the ZDC detector #46570

Merged

abdoulline mentioned this pull request Nov 18, 2024

ZDC Phase2 HcalTPChannelParameters update to silence multiple warnings #46721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modification to move ZDC trigger spacing to TP channel parameters conditions. #46537

Modification to move ZDC trigger spacing to TP channel parameters conditions. #46537

hjbossi commented Oct 28, 2024

cmsbuild commented Oct 28, 2024 •

edited

Loading

cmsbuild commented Oct 28, 2024

JHiltbrand commented Oct 29, 2024

hjbossi commented Oct 29, 2024 •

edited

Loading

cmsbuild commented Oct 29, 2024

cmsbuild commented Oct 29, 2024

mandrenguyen commented Oct 29, 2024

perrotta left a comment

perrotta Oct 29, 2024

perrotta Oct 29, 2024

hjbossi Oct 29, 2024

perrotta Oct 29, 2024

hjbossi Oct 29, 2024

hjbossi commented Oct 29, 2024

perrotta commented Oct 31, 2024

perrotta commented Oct 31, 2024

cmsbuild commented Oct 31, 2024

perrotta commented Oct 31, 2024

mandrenguyen commented Oct 31, 2024

mandrenguyen commented Oct 31, 2024

aloeliger commented Nov 1, 2024

cmsbuild commented Nov 1, 2024

mmusich Nov 15, 2024

perrotta Nov 16, 2024

hjbossi Nov 16, 2024

mmusich Nov 16, 2024

perrotta Nov 16, 2024

abdoulline commented Nov 16, 2024 •

edited

Loading

yildirayk commented Nov 16, 2024

		int weight = tpParam->getauxi1();
		int factorGeVPerCount = tpParam->getauxi2();

		int lutId = getLUTId(cell);
		int lutId_ootpu = lutId + sizeZDC_;

Modification to move ZDC trigger spacing to TP channel parameters conditions. #46537

Modification to move ZDC trigger spacing to TP channel parameters conditions. #46537

Conversation

hjbossi commented Oct 28, 2024

PR description:

PR validation:

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:

cmsbuild commented Oct 28, 2024 • edited Loading

cmsbuild commented Oct 28, 2024

JHiltbrand commented Oct 29, 2024

hjbossi commented Oct 29, 2024 • edited Loading

cmsbuild commented Oct 29, 2024

cmsbuild commented Oct 29, 2024

mandrenguyen commented Oct 29, 2024

perrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjbossi commented Oct 29, 2024

perrotta commented Oct 31, 2024

perrotta commented Oct 31, 2024

cmsbuild commented Oct 31, 2024

RelVals-INPUT

Comparison Summary

perrotta commented Oct 31, 2024

mandrenguyen commented Oct 31, 2024

mandrenguyen commented Oct 31, 2024

aloeliger commented Nov 1, 2024

cmsbuild commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdoulline commented Nov 16, 2024 • edited Loading

yildirayk commented Nov 16, 2024

cmsbuild commented Oct 28, 2024 •

edited

Loading

hjbossi commented Oct 29, 2024 •

edited

Loading

abdoulline commented Nov 16, 2024 •

edited

Loading