Gtt jet finding geometry readout - #50908
Conversation
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50908/49269
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50908/49271
|
|
A new Pull Request was created by @ccahoughton for master. It involves the following packages:
@BenjaminRS, @cmsbuild, @quinnanm can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
+1 Size: This PR adds an extra 48KB to repository Comparison SummarySummary:
|
|
Hi Connor - to let you know. PR #49003 will soon be closed and this also modified L1Trigger/DemonstratorTools/test/gtt/createFirmwareInputFiles_cfg.py. So I suggest we wait until that one is in and we will ask you to rebase on-top of that PR. In the mean time we can review the other files modified. |
quinnanm
left a comment
There was a problem hiding this comment.
Thank you for this PR! Just a few comments so far.
| // ----------constants, enums and typedefs --------- | ||
| static constexpr double MAX_Z0 = 30.; | ||
| static constexpr double MAX_ETA = 8.; | ||
| static constexpr double MAX_ETA = 2 * M_PI; |
| int minTrkJetTrackMultiplicity_, | ||
| float minTrkJetpT_) { | ||
| ofstream FirmwareGeometryFile; | ||
| FirmwareGeometryFile.open("./FirmwareGeometryFile.vh"); |
There was a problem hiding this comment.
May be useful add a check here to make sure the file is open
There was a problem hiding this comment.
Okay, I put in a simple solution:
if (FirmwareGeometryFile.is_open()) {
...
...
...
} else {
std::cout << "Failed to open firmware geometry export file" << std::endl;
}
Let me know if you had something else in mind!
| //out as bit words to the geometry file | ||
| for (int i = 0; i < etaBins_; i++) { | ||
| unsigned int EtaBitInt = DoubleToBit( | ||
| eta_bin_centers[i], l1t::TkJetWord::TkJetBitWidths::kGlbEtaSize, l1t::TkJetWord::MAX_ETA * pow(2, -13)); |
There was a problem hiding this comment.
Should l1t::TkJetWord::MAX_ETA * pow(2, -13) be derived from kGlbEtaSize to avoid magic numbers? For example: (2 * MAX_ETA) / (1 << kGlbEtaSize)
There was a problem hiding this comment.
Yes, I will change it.
| DoubleToBit(double(mzb.clusters[j].eta), | ||
| TkJetWord::TkJetBitWidths::kGlbEtaSize, | ||
| 2 * TkJetWord::MAX_ETA / (1 << TkJetWord::TkJetBitWidths::kGlbEtaSize)); | ||
| // TkJetWord::MAX_ETA / (1 << TkJetWord::TkJetBitWidths::kGlbEtaSize)); |
There was a problem hiding this comment.
could this comment be removed?
There was a problem hiding this comment.
Yes, I will remove it.
|
Milestone for this pull request has been moved to CMSSW_20_0_X. Please open a backport if it should also go in to CMSSW_17_0_X. |
|
Just checking, is this PR intended for Phase 2 (20_1_X) or Run 3 legacy (17_0_X) or both? |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50908/49647
|
|
Pull request #50908 was updated. @BenjaminRS, @cmsbuild, @quinnanm can you please check and sign again. |
|
this is intended for Phase 2 (20_1_X) |
|
Thanks. Then no further action is needed for this PR regarding the target branch. |
|
Milestone for this pull request has been moved to CMSSW_20_1_X. Please open a backport if it should also go in to CMSSW_20_0_X. |

PR description:
PR validation: