-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Use Pedestal Width for ZS During HBHE TP Formation #49724
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
base: master
Are you sure you want to change the base?
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49724/47306
|
|
A new Pull Request was created by @JHiltbrand for master. It involves the following packages:
@Alejandro1400, @BenjaminRS, @JanChyczynski, @arunhep, @atpathak, @cmsbuild, @perrotta, @quinnanm can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
-1 Failed Tests: UnitTests Failed Unit TestsI found 1 errors in the following unit tests: ---> test test-das-selected-lumis had ERRORS Comparison SummarySummary:
|
|
please test |
|
+1 Size: This PR adds an extra 16KB to repository Comparison SummarySummary:
|
| // The number of pedestal widths for ZS is packed into the third byte | ||
| // of the 32 bit auxi1 variable from HcalTPParameters | ||
| // Assume a fixed-point SF of 1 / 16 to convert to final number of widths | ||
| double nPedWidthsForZSfromDB = ((aux1 & 0xFF0000u) >> 16) / 16.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several assumptions here:
The value extracted from this condition parameter is assumed to be fixed-point with a scale factor of 16 to allow for a final fractional value to be used as the number of width
When do you think that this will be finalized, and a candidate condition can be tested with this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way: shouldn't it be
| double nPedWidthsForZSfromDB = ((aux1 & 0xFF0000u) >> 16) / 16.0; | |
| double nPedWidthsForZSfromDB = ((aux1 >> 16) & 0xFF0000u) / 16.0; |
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way: shouldn't it be
instead?
I think with your suggestion, one would need to AND with 0xFFu, since the shift by 16 bits first moved what was in the third byte to become the lowest byte. But perhaps I am missing something ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several assumptions here: The value extracted from this condition parameter is assumed to be fixed-point with a scale factor of 16 to allow for a final fractional value to be used as the number of width When do you think that this will be finalized, and a candidate condition can be tested with this PR?
Indeed, some assumptions here..., which is the theme with our recent campaign of packing things into the bytes of unused auxiliary words (e.g. #49012). The onus is on HCAL to keep track of how and where each byte is prepared/used correctly.
I believe we have determined recently (@akhukhun can correct me) that a realistic value for the number of pedestal widths to use would 1.31 (i.e. a third byte equal to 0x15), so a candidate condition could be produced to test with this PR. I am unfamiliar with testing PRs with specific candidate conditions, what is the best way to proceed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right: I meant
| double nPedWidthsForZSfromDB = ((aux1 & 0xFF0000u) >> 16) / 16.0; | |
| double nPedWidthsForZSfromDB = ((aux1 >> 16) & 0xFFu) / 16.0; |
In my mind the logic would be: first skip the first two bytes, and then select the following one. But at the end, I agree with you that it may end up with assigning the content of the same byte, indeed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we have determined recently (@akhukhun can correct me) that a realistic value for the number of pedestal widths to use would 1.31 (i.e. a third byte equal to
0x15), so a candidate condition could be produced to test with this PR. I am unfamiliar with testing PRs with specific candidate conditions, what is the best way to proceed ?
This is a test that you can do yourself, outside of the automatic PR tests, and report about it in the PR thread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we have determined recently (@akhukhun can correct me) that a realistic value for the number of pedestal widths to use would 1.31 (i.e. a third byte equal to
0x15), so a candidate condition could be produced to test with this PR. I am unfamiliar with testing PRs with specific candidate conditions, what is the best way to proceed ?This is a test that you can do yourself, outside of the automatic PR tests, and report about it in the PR thread
Hi @perrotta,
How sophisticated of a test is desired ? I am imagining having a test tag of the HcalTPParameter condition in conddb, and setting up a basic cmsRun job that runs the affected code to show that an expected value for number of pedestal widths comes out. Or are you looking for physics impact (if so, see my slides I linked in the PR main text) ? Or something else entirely ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How sophisticated of a test is desired ? I am imagining having a test tag of the
HcalTPParametercondition in conddb, and setting up a basiccmsRunjob that runs the affected code to show that an expected value for number of pedestal widths comes out. Or are you looking for physics impact (if so, see my slides I linked in the PR main text) ? Or something else entirely ?
I think that for the PR it may be sufficient to " test tag of the HcalTPParameter condition in conddb, and setting up a basic cmsRun job that runs the affected code to show that an expected value for number of pedestal widths comes out"
As you write, you were already showing the physics impact of this development, and I assume that L1T will take care of it for their signature.
| std::unique_ptr<HcalPulseContainmentManager> pulseCorr_; | ||
| bool overrideDBweightsAndFilterHB_ = false; | ||
| bool overrideDBweightsAndFilterHE_ = false; | ||
| double nPedWidthsForZS_ = 0.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ped and pedwidth are both float: why to make nPedWidthsForZS_, nPedWidthsForZSfromDB and nPedWidthsForZSfinal as double?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understandable, I can make them float.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to float.
|
Hi @perrotta, I have performed a basic test to verify the behavior of this code and extracting information from the
Hopefully this test is meaningful to see how the code works with an actual test tag for |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49724/47499 |
01c0d5a to
ffc83f3
Compare
|
Pull request #49724 was updated. @Alejandro1400, @BenjaminRS, @JanChyczynski, @arunhep, @atpathak, @cmsbuild, @perrotta, @quinnanm can you please check and sign again. |
|
please test |
|
+1 Size: This PR adds an extra 28KB to repository Comparison SummarySummary:
|
|
+alca
|
PR description:
This PR adds functionality to perform zero suppression during trigger primitive formation for the HCAL barrel and endcap. The ZS threshold is configured as some amount of pedestal widths (per-channel), where the widths are simply taken from the conditions database. The amount of widths to use is anticipated to be stored in the third byte of the
auxi1parameter of theHcalTPParameterscondition. The value extracted from this condition parameter is assumed to be fixed-point with a scale factor of 16 to allow for a final fractional value to be used as the number of widths. E.g. a third byte value of 21 results in a number of widths of 1.3125.Historically, the third byte (and all) of
auxi1ofHcalTPParametershas been unused and set to 0 (only recently as of PR #49012 has the first and second bytes been used). Thus, with the default value of 0 for the third byte, the aforementioned functionality is not activated and no changes are expected in TPs or LUT XMLs. Once the third byte of theHcalTPParameterauxi1parameter is filled, the new functionality is activated. Python configuration overrides are also made available, but they are also off by default.PR validation:
Ran the local LUT XML production workflow and manipulated the third byte of
auxi1ofHcalTPParametersto yield changes in the LUT consistent with the effect of zero suppression.A talk was given to L1T DPG over-viewing this development with example impact on TPs for a number of pedestal widths of 3
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 itself, but should be backported to 16_0_X
ATTN: @akhukhun