-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Phase 2 GTT Fix ptbits z0bits of GTT ObjectWords #44334
Phase 2 GTT Fix ptbits z0bits of GTT ObjectWords #44334
Conversation
…s, avoid truncating float bits from ap_(u)fixed
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44334/39367
|
A new Pull Request was created by @NJManganelli for master. It involves the following packages:
@aloeliger, @epalencia, @cmsbuild can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5eb20a/37952/summary.html Comparison SummarySummary:
|
@NJManganelli I'll sign this, but real quickly, For my edification what does the addition of |
Hi @aloeliger , right, so the range call selects all bits from the LSB to MSB. The problem with the old code was, the to_uint() call was effectively a type-aware cast of the ap_(u)fixed data, so it only grabbed the actual magnitude bits (So the top 6 of 15, for vertex z0, as an example). See the last column of the attached, one before and one after the fix, which shows the stored output from these calls on the rightmost column (the correct values are in the middle column, which select the appropriate subset of bits from the full Word in the left column; the 9 least significant bits for the float are truncated off, leaving the two-three highest bits only). See Range Select |
+l1 |
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. @rappoccio, @sextonkennedy, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
The methods ptBits() and z0Bits() of the TkJetWord.h and VertexWord.h classes attempt to return the bits for ap_(u)fixed datatypes, but the direct call to to_uint() truncated the float bits, leaving only the magnitude bits. These methods were not found to be in use anywhere in CMSSW, except now for adding GTT objects to L1Nano. This fix updates the methods, and they appropriately return all the magnitude + float bits, which enables full agreement in L1Nano for Vertices and TrackJets. scram build updateclassversion does not produce an updated class version/hash (nor direct usage of edmCheckClassVersion -g ...).
PR validation:
This PR passes
scram b
scram b code-checks
scram b code-format
scram b updateclassversion
Used to generate GTT objects in L1Nano format for ~3000 events
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:
port of cms-l1t-offline#1217