Follow up to PR31722 (#614) #615
Conversation
ericcano
force-pushed
the
cms-patatrack-cmssw-614
branch
from
c0fed97
to
9827fb7
Compare
…sw#614) Used accessors in all cases. This commit does not change the memory layout of the class. Also fixed C-style casts (reinterpret casts are needed here). cms-sw#31722 (comment) cms-sw#31722 (comment)
Also removed addition of transposed matrix by directly copying the needed elements. cms-sw#31722 (comment) cms-sw#31722 (comment)
Also introduced a computation simplification suggested in: cms-sw#31722 (comment)
Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>
ericcano
force-pushed
the
cms-patatrack-cmssw-614
branch
from
558b5d8
to
96d121f
Compare
Also made the type explicit. cms-patatrack#615 (comment)
| HitContainer hitIndices; | ||
| HitContainer detIndices; | ||
| HitContainer hitIndices_; | ||
| HitContainer detIndices_; |
There was a problem hiding this comment.
I am not fully convinced of this encapsulations of SoAs.
It is just mere orthography. Semantics has not changed as both const and non-const accessors have been provided.
What are we encpsulating here?
The implementation encapsulation is done in the SoA itself.
Given that it is highly probable that all this will be modified again once the eventual final SoA model will be deployed (together with the Heterogeneous framework) I suggest to keep an open issue on Orthography of SoA
and leave for the time being my choice of orthography for my design of SoAs....
There was a problem hiding this comment.
So please split the changes in the Arithmetic of the fit (that is supposed (I hope) a performance improvement) from this esthetic changes. (also because we want to profile them independently)
There was a problem hiding this comment.
This commit just ensures we have a class with only one of each public, protected and private section. So indeed entirely cosmetic, but mandated by coding rules.
Computations and code style are already in different commits, so I guess you meant splitting this PR?
There was a problem hiding this comment.
yes, spitting this PR:
one cosmetics
one computational performance
|
As requested by @VinInn, I close this PR to split it into a performance PR on one side and code cleanliness PR on the other. |
Also made the type explicit. cms-patatrack#615 (comment)
Also made the type explicit. cms-patatrack#615 (comment)
|
I am now working on the pixel producers. I copied the version in here. Please do not commit them anymore. |
Also made the type explicit. cms-patatrack#615 (comment)
Also made the type explicit. cms-patatrack#615 (comment)
PR description:
PR validation:
if this PR is a backport please specify the original PR and why you need to backport that PR:
Before submitting your pull requests, make sure you followed this checklist: