L1 tracking update #47067
Open
L1 tracking update #47067
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47067/43234
|
cmsbuild
added
code-checks-rejected
code-checks-pending
and removed
code-checks-pending
new-package-pending
code-checks-rejected
labels
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47067/43235
|
tschuh
force-pushed
the
L1TK-PR-14_2_0_pre4
branch
from
94ae2d7 to
e2dc5ef
Compare
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47067/43238
|
ci workflow deleted from pr. data folder deleted.
|
+Upgrade |
makortel
reviewed
Jan 14, 2025
There was a problem hiding this comment.
The core part itself (DataFormats/WrappedStdDictionaries/src/classes_def.xml) is fine, but I took a very cursory look to the other code.
I noticed two widespread patterns that should be improved:
- holding copies of
edm::ParameterSetand parsing them during event processingParameterSetcopies take memory (they are not lightweight)- parsing
ParameterSetis not fast (it's not super-slow either, but not intended to be done during data processing)
- function-
statics (even ifconst) that are initialized based on the state of some object- It was not straightforward to figure out whether those
staticconstants are effectively build-time-defined constants, or depend on some runtime-dependent value(s) such as coming from the EventSetup. In the latter case this pattern effectively prevents these values changing between IOVs (or testing two payloads with different values in the same job), but in the way the situation can only be discovered from the physics results. - There was one non-
constfunction static that is a thread-safety issue
- It was not straightforward to figure out whether those
Comment on lines
+37
to
+40
| // increment of Process | ||
| inline constexpr Process operator++(Process p) { return Process(+p + 1); } | ||
| // increment of Variable | ||
| inline constexpr Variable operator++(Variable v) { return Variable(+v + 1); } |
There was a problem hiding this comment.
I could imagine prefix increment of ++a not incrementing a to lead to a confusion by a future reader of the code.
Comment on lines
10
to
13
| typedef edm::mpl::Vector<ChannelAssignmentRcd> RcdsDataFormats; | ||
|
|
||
| // record of trklet::DataFormats | ||
| class DataFormatsRcd : public edm::eventsetup::DependentRecordImplementation<DataFormatsRcd, RcdsDataFormats> {}; |
There was a problem hiding this comment.
Could ChannelAssignmentRcd be used directly, since DataFormatsRcd doesn't depend on anything else? Or is DataFormatsRcd foreseen to depend on additional Records?
There was a problem hiding this comment.
No it only depends on ChannelAssignmentRcd. How does one use a Rcd directly?
There was a problem hiding this comment.
You can just replace all uses of DataFormatsRcd with ChannelAssignmentRcd.
| // number of bits used to represent layer id [barrel: 0-5, discs: 6-10] | ||
| int widthLayerId_; | ||
| // TM parameter | ||
| edm::ParameterSet pSetTM_; |
There was a problem hiding this comment.
It would be great if copying edm::ParameterSet could be avoided.
| int iPhiSec, | ||
| int iEtaReg) | ||
| : settings_(settings), | ||
| stubs_(stubs), |
There was a problem hiding this comment.
Suggested change
| stubs_(stubs), | |
| stubs_(std::move(stubs)), |
to avoid a copy.
Comment on lines
87
to
88
| stubsConst_(std::vector<const Stub*>()), | ||
| bestStubs_(std::unordered_set<const Stub*>()), |
There was a problem hiding this comment.
These have the same effect as default constructor, and are thus not needed.
Suggested change
| stubsConst_(std::vector<const Stub*>()), | |
| bestStubs_(std::unordered_set<const Stub*>()), |
Comment on lines
279
to
280
| static const DataFormat& dfInv2R = dataFormats_->format(Variable::inv2R, Process::ht); | ||
| static const DataFormat& dfPhiT = dataFormats_->format(Variable::phiT, Process::ht); |
| void produce(StreamID, Event&, const EventSetup&) const override; | ||
| void beginRun(const Run&, const EventSetup&) override; | ||
| void produce(Event&, const EventSetup&) override; | ||
| void endJob() {} |
There was a problem hiding this comment.
Suggested change
| void endJob() {} |
| @@ -50,25 +54,70 @@ namespace tt { | |||
| EDPutTokenT<StubAssociation> putTokenSelection_; | |||
| // Setup token | |||
| ESGetToken<Setup, SetupRcd> esGetTokenSetup_; | |||
| // | |||
| ParameterSet pSet_; | |||
There was a problem hiding this comment.
It would be great to avoid holding a copy of the edm::ParameterSet.
| static constexpr bool intimeOnly = true; | ||
| static constexpr bool chargedOnly = true; | ||
| static constexpr bool stableOnly = false; | ||
| static const double maxEta = asinh((maxZT_ + maxZ0_) / setup_->chosenRofZ()); |
Comment on lines
139
to
140
| StubAssociation reconstructable(pSet_, setup_); | ||
| StubAssociation selection(pSet_, setup_); |
There was a problem hiding this comment.
Parsing edm::ParameterSet within event processing is slow
|
I saw there are 3 workflows with trigger differences, so here is the only with >1 trigger differing: Though the stats are very low (@aloeliger @Moanwar can we have e.g. this wf with 100 events in future) there are more accepted events in three hadronic seeds. This is a bit suspicious for pure sw/fw syncing imo. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47067/43470
|
|
Pull request #47067 was updated. @Dr15Jones, @Moanwar, @aloeliger, @antoniovilela, @civanch, @cmsbuild, @davidlange6, @epalencia, @fabiocos, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @rappoccio, @smuzaffar, @srimanob, @subirsarkar can you please check and sign again. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47067/43488
|
|
Pull request #47067 was updated. @Dr15Jones, @Moanwar, @aloeliger, @antoniovilela, @civanch, @cmsbuild, @davidlange6, @epalencia, @fabiocos, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @rappoccio, @smuzaffar, @srimanob, @subirsarkar can you please check and sign again. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates mainly the modules
in order to sync s/w emulation of L1 tracking with current f/w. Some name changes have also been propagated to additional modules.
The code changes of this PR did pass review of L1 tracking community checking for L1 tracking performance, code formats, etc.