-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add initial null value for model result in calo summary #42376
Add initial null value for model result in calo summary #42376
Conversation
please test for CMSSW_13_3_CPP20_X |
Thanks @aloeliger ! Don't worry about warnings in hls ( |
@iarspider I'll try to take a look at the warnings in |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42376/36404
|
A new Pull Request was created by @aloeliger (Andrew Loeliger) for master. It involves the following packages:
@epalencia, @aloeliger can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
-1 Failed Tests: RelVals AddOn RelVals
Expand to see more relval errors ...
AddOn Tests
Expand to see more addon errors ... |
Hmm, this shouldn't be able to fail any relVals, it doesn't run in any current path. |
This seems to be the common error. This doesn't seem like something I have introduced. |
@aloeliger ah, sorry, should've worded that explicitly in #41794: the only thing left to fix are these warnings in
|
And RelVal/test failures are expected, since CPP20 IBs are broken by other issues. |
Okay, I'm looking at these, but they appear to be triply nested |
After reading further, I'm not even sure what we consider unity here given that the LUT is used in some bitwise operation in the towers themselves (i.e. is unity 1? doesn't make sense from a bit perspective. Is unity 1 for all bits? Is it 0?). I will be meeting with the calo trigger group today in any case, I'll try to sort out an answer from the experts then. |
+l1
|
This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6a7984/33958/summary.html Comparison SummarySummary:
|
+1 |
PR description:
Adds an initial (null) value for the
modelResult
CICADA variable in L1TCaloSummary in response to #41794@iarspider This should(?) hopefully silence some of the warning related to
L1TCaloSummary.cc
, but I can't do anything about build warnings that may belong to theap_fixed
type itself. I will look at the L1TCaloLayer1 plugin soonPR validation:
All code compiles, passes code-checks, and has had code-formatting applied. In theory this should be a trivial technical fix that again shouldn't be able to mess anything else up.
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 not a backport, or tied into any existing workflows.