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
[L1T] Phase-2, Update Track Selection Emulation to Match Firmware #40096
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40096/33079
|
A new Pull Request was created by @cecilecaillol for master. It involves the following packages:
@rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
-1 Failed Tests: RelVals-INPUT RelVals-INPUTThe relvals timed out after 4 hours.
Comparison SummarySummary:
|
please test (relvals timed out) |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-06bf60/29104/summary.html Comparison SummarySummary:
|
+l1 |
@AdrianoDee, @srimanob Could you please check and sign? We would like to have it in 12_6_0_pre5 to rebase our branch. It is a small PR. |
ap_uint<TrackBitWidths::kPtSize> ptEmulationBits = t.getTrackWord()( | ||
TTTrack_TrackWord::TrackBitLocations::kRinvMSB - 1, TTTrack_TrackWord::TrackBitLocations::kRinvLSB); | ||
ap_ufixed<TrackBitWidths::kPtSize, TrackBitWidths::kPtMagSize> ptEmulation; | ||
ptEmulation.V = ptEmulationBits.range(); |
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.
Where are these ptEmulationBits
and ptEmulation
actually used in this method?
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.
@aperloff Can you please answer this question?
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.
The information comes in as an N bit ap_uint, but it's really representing the charge and pt. The Nth bit is the charge and bits N-1 down to 0 are the pt. So the N-1 LSBs must be converted into an ap_ufixed. ptEmulationBits
is the original set of bits in an ap_uint. ptEmulation
is the more useful ap_fixed. ptEmulationBits
is used to set the value of ptEmulation
, which is then used in the return statement.
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.
Sorry to insist but, while I understood the what thanks to your explanation, I still don't see the where. Unless there's some magic I don't see, ptEmulation
doesn't participate to the return
(while it does is some other operators above)
return std::abs(v.z0() - fixedTkZ0.to_double()) <= deltaZMax_[etaIndex];
We have only v
coming from the inputs and fixedTkZ0
, that makes sense since, as far as I have understood, this method checks the compatibility between a vertex and the track vertex.
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.
@AdrianoDee It's used here:
if (processEmulatedTracks_ && kinSelEmu(track) && chi2SelEmu(track)) { |
That Selector is defined as part of an AND selector, and the instantiated AND selector is used here in L583.
Hope that clears things up.
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.
ap_uint<TrackBitWidths::kPtSize> ptEmulationBits = t.getTrackWord()( | |
TTTrack_TrackWord::TrackBitLocations::kRinvMSB - 1, TTTrack_TrackWord::TrackBitLocations::kRinvLSB); | |
ap_ufixed<TrackBitWidths::kPtSize, TrackBitWidths::kPtMagSize> ptEmulation; | |
ptEmulation.V = ptEmulationBits.range(); |
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.
This should be directly committable.
@@ -276,7 +276,7 @@ class TTTrack_TrackWord { | |||
return reco::deltaPhi(globalPhi, (sector * sectorWidth)); | |||
} | |||
|
|||
private: | |||
public: | |||
// ----------private member functions -------------- |
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.
The comment could be removed or fixed.
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.
done
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40096/34429
|
Pull request #40096 was updated. @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @aloeliger, @cecilecaillol can you please check and sign again. |
please test |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-06bf60/31024/summary.html Comparison SummarySummary:
|
+upgrade
Comments addressed, tests run
…________________________________
Da: cmsbuild ***@***.***>
Inviato: venerdì 3 marzo 2023 13:27
A: cms-sw/cmssw ***@***.***>
Cc: Adriano Di Florio ***@***.***>; Mention ***@***.***>
Oggetto: Re: [cms-sw/cmssw] [L1T] Phase-2, Update Track Selection Emulation to Match Firmware (PR #40096)
+1
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-06bf60/31024/summary.html
COMMIT: 2d00a3e<2d00a3e>
CMSSW: CMSSW_13_1_X_2023-03-02-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40096/31024/install.sh to create a dev area with all the needed externals and cmssw changes.
Comparison Summary
Summary:
* You potentially added 11 lines to the logs
* Reco comparison results: 5 differences found in the comparisons
* DQMHistoTests: Total files compared: 49
* DQMHistoTests: Total histograms compared: 3529699
* DQMHistoTests: Total failures: 6
* DQMHistoTests: Total nulls: 0
* DQMHistoTests: Total successes: 3529671
* DQMHistoTests: Total skipped: 22
* DQMHistoTests: Total Missing objects: 0
* DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
* Checked 213 log files, 164 edm output root files, 49 DQM output files
* TriggerResults<https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_13_1_X_2023-03-02-2300+06bf60/55820/triggerResults>: no differences found
—
Reply to this email directly, view it on GitHub<#40096 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEA6IGWZLMMNHQHBEYSEBD3W2HPS7ANCNFSM6AAAAAASDFVN44>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
+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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Updates the TrackSelection outputs to better match the firmware. Care is taken to make sure that no undigitization rounding errors are incurred before the comparisons selecting the tracks are made.
PR validation:
The outputs of this PR now match the firmware outputs. Also the standard scram checks were performed
Porting of local cms-l1t-offline#1055