-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Integrating VectorHits reconstruction for the Phase2 OT - reprise #31314
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
Changes from 95 commits
1d33be2
9643a44
3bc0dde
09136ad
e0a73fd
3e0523b
00ad9bb
e652383
5ad296d
05858c2
0ffda00
7c1b911
872a25c
ac8f3be
35bc979
672156a
2dd7bf8
d5c6cb4
428127c
236113f
ee3a194
c9eace1
8bdb8a4
9e38c4c
4391753
7b018f7
cb8050b
4e69b00
728f0d0
8b43243
472b65a
7725ff0
ac77617
5f6110a
b775266
142acec
ce1a3ea
5903788
efcebde
727cb46
17bbbd6
d06cd10
9e1ca77
6fa2229
2536365
1009a20
b17261d
01114c0
2cdd767
ccf9e94
33bbeff
758d273
754d08c
403ac04
7c64d65
424df27
9109e2e
0aba3e5
90cf48a
6b9b4f2
61599bc
18a03b3
8bf7d15
45ec1f2
17a2e02
f149ea2
ccc74b6
999d80b
92f2c7e
68ea0ce
ba5e5d4
2d15513
66e765d
c3357eb
e9b6828
580200d
16e51bb
cfd6018
74cf64c
f1ce21b
ec1a12e
2c87639
24d84b9
9ac1ebe
03f9495
5c42b05
867caf2
1edd84d
d8b3b31
35d4bf7
b4fe5a6
38225d6
4774710
b6b9e9e
1e9919f
1fea02b
946eb90
359a1bc
41ed2ad
e58adab
2689ca8
671803f
93a827a
3b4b4d2
224b3a9
41ff72e
d952d3a
7d164db
72ba9db
6dcc8c4
a82f984
6019571
3300a89
988c5a3
53aad7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import FWCore.ParameterSet.Config as cms | ||
|
|
||
| # This modifier is for activating Vector Hits reconstruction in the Phase II OT | ||
|
|
||
| vectorHits = cms.Modifier() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -298,6 +298,24 @@ def condition_(self, fragment, stepList, key, hasHarvest): | |
| '--procModifiers': 'trackingMkFit' | ||
| } | ||
|
|
||
| # Vector Hits workflows | ||
| class UpgradeWorkflow_vectorHits(UpgradeWorkflow): | ||
| def setup_(self, step, stepName, stepDict, k, properties): | ||
| stepDict[stepName][k] = merge([{'--procModifiers': 'vectorHits'}, stepDict[step][k]]) | ||
| def condition(self, fragment, stepList, key, hasHarvest): | ||
| return fragment=="TTbar_14TeV" and '2026' in key | ||
| upgradeWFs['vectorHits'] = UpgradeWorkflow_vectorHits( | ||
| steps = [ | ||
| 'Reco', | ||
| ], | ||
| PU = [ | ||
| 'Reco', | ||
| ], | ||
| suffix = '_vectorHits', | ||
| offset = 0.9, | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kpedro88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just as a sanity check, it's not just me, the cms-bot/jenkins cmdLog for the .9 workflow does not have it either
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I must have broken it when doing the last rebase as there were conflicts in this file. Working to fix it...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Phase2 workflows use the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That fixes it, thanks! That probably also means that it never actually worked in the first place, sorry! |
||
|
|
||
|
|
||
| # Patatrack workflows | ||
| class UpgradeWorkflowPatatrack(UpgradeWorkflow): | ||
| def condition(self, fragment, stepList, key, hasHarvest): | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,168 @@ | ||||||||||
| #ifndef DataFormats_TrackerRecHit2D_VectorHit_h | ||||||||||
| #define DataFormats_TrackerRecHit2D_VectorHit_h | ||||||||||
|
|
||||||||||
| /** \class VectorHit | ||||||||||
| * | ||||||||||
| * 4-parameter RecHits for Phase2 Tracker (x,y, dx/dz, dy/dz) | ||||||||||
| * | ||||||||||
| * $Date: 2015/03/30 $ | ||||||||||
| * \author Erica Brondolin | ||||||||||
| * | ||||||||||
| */ | ||||||||||
|
|
||||||||||
| #include "DataFormats/TrackerRecHit2D/interface/BaseTrackerRecHit.h" | ||||||||||
| #include "DataFormats/TrackerRecHit2D/interface/VectorHit2D.h" | ||||||||||
| #include "DataFormats/TrackerRecHit2D/interface/OmniClusterRef.h" | ||||||||||
|
|
||||||||||
| #include "DataFormats/Common/interface/DetSetVector.h" | ||||||||||
| #include "DataFormats/Common/interface/DetSetVectorNew.h" | ||||||||||
|
|
||||||||||
| #include "DataFormats/Phase2TrackerCluster/interface/Phase2TrackerCluster1D.h" | ||||||||||
| #include "DataFormats/GeometryVector/interface/LocalVector.h" | ||||||||||
| #include "Geometry/CommonDetUnit/interface/PixelGeomDetUnit.h" | ||||||||||
|
|
||||||||||
| #include "DataFormats/TrackingRecHit/interface/KfComponentsHolder.h" | ||||||||||
|
|
||||||||||
| #include "DataFormats/TrackerRecHit2D/interface/TkCloner.h" | ||||||||||
|
|
||||||||||
| class VectorHit final : public BaseTrackerRecHit { | ||||||||||
| public: | ||||||||||
| typedef OmniClusterRef::Phase2Cluster1DRef ClusterRef; | ||||||||||
|
|
||||||||||
| VectorHit() : thePosition(), theDirection(), theCovMatrix(), theDimension(0) { setType(bad); } | ||||||||||
|
|
||||||||||
| VectorHit(const VectorHit& vh); | ||||||||||
|
|
||||||||||
| VectorHit(const GeomDet& idet, | ||||||||||
| const LocalPoint& posInner, | ||||||||||
| const LocalVector& dir, | ||||||||||
| const std::array<std::array<float, 4>, 4> covMatrix, | ||||||||||
|
||||||||||
| const float chi2, | ||||||||||
| OmniClusterRef const& lower, | ||||||||||
| OmniClusterRef const& upper, | ||||||||||
| const float curvature, | ||||||||||
| const float curvatureError, | ||||||||||
| const float phi); | ||||||||||
|
|
||||||||||
| VectorHit(const GeomDet& idet, | ||||||||||
| const VectorHit2D& vh2Dzx, | ||||||||||
| const VectorHit2D& vh2Dzy, | ||||||||||
| OmniClusterRef const& lower, | ||||||||||
| OmniClusterRef const& upper, | ||||||||||
| const float curvature, | ||||||||||
| const float curvatureError, | ||||||||||
| const float phi); | ||||||||||
|
|
||||||||||
| ~VectorHit() override; | ||||||||||
|
||||||||||
| ~VectorHit() override; | |
| ~VectorHit() override = default; |
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.
Outdated
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.
should/can be static?
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.
as is, it should be a free function (no need for namespace either)
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 function is really only just to compare a different VH to this one, so I changed it to compare to this.
JanFSchulte marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
| //if det is present pose&err are available as well. | |
| //if det() is not present (null) the hit ihas been read from file and not updated | |
| //if det is present pos&err are available as well. | |
| //if det() is not present (null) the hit has been read from file and not updated |
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.
Outdated
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.
| AlgebraicVector parameters() const override; |
this is marked as obsolete in the base and is itself not purely virtual ==> remove
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.
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.
given class VectorHit final, is a new virtual method still useful at this level?
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.
Not really, I don't know with this was made virtual. Removed it.
Outdated
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.
return by ref or do we expect some compiler optimization magic (RVO)?
Actually, is this method even needed?
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 method is needed to avoid creating AlgebraicSymMatrix44 in the copy constructor and is currently only used for this purpose. To the outside world there is still the other parametersError method that creates and returns the parametersError object.
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 copy constructor should be able to use vh.theCovMatrix
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.
Function is gone.
Outdated
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.
| AlgebraicSymMatrix parametersError() const override; |
this is marked as obsolete in the base and is itself not purely virtual ==> remove
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.
Outdated
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.
need to be virtual?
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.
I don't think so, changed.
JanFSchulte marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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 method returns the delta in global coordinates | |
| //This method returns the delta (direction of the segment/stub) in global coordinates |
I'm not sure delta is a common variable, even internally in the analyzer this is called dirVH.
Perhaps even rename the method to globalDirectionVH ?
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.
Good point. I renamed it to globalDirection. I think as a member function it is clear that it refers to the direction of the VH.
Outdated
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 projection matrix relates the trajectory state parameters to the segment parameters(). | |
| AlgebraicMatrix projectionMatrix() const override; |
this is marked as obsolete in the base and is itself not purely virtual ==> remove
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.
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.
what is this non const method that return something by value????
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.
Not having it leads to compilation errors because the concrete implementation of the non-const function seems to needed, without it I get the following errors:
/uscms_data/d3/jschulte/VH/new/CMSSW_11_2_X_2020-09-03-1100/src/DataFormats/TrackerRecHit2D/interface/VectorHit.h:52:65: error: invalid new-expression of abstract class type 'VectorHit' VectorHit* clone() const override { return new VectorHit(*this); }
^
/uscms_data/d3/jschulte/VH/new/CMSSW_11_2_X_2020-09-03-1100/src/DataFormats/TrackerRecHit2D/interface/VectorHit.h:28:7: note: because the following virtual functions are pure within 'VectorHit': class VectorHit final : public BaseTrackerRecHit {
^~~~~~~~~
In file included from /uscms_data/d3/jschulte/VH/new/CMSSW_11_2_X_2020-09-03-1100/src/DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:4, from /uscms_data/d3/jschulte/VH/new/CMSSW_11_2_X_2020-09-03-1100/src/DataFormats/TrackerRecHit2D/interface/BaseTrackerRecHit.h:4, from /uscms_data/d3/jschulte/VH/new/CMSSW_11_2_X_2020-09-03-1100/src/DataFormats/TrackerRecHit2D/interface/VectorHit.h:13, from /uscms_data/d3/jschulte/VH/new/CMSSW_11_2_X_2020-09-03-1100/src/DataFormats/TrackerRecHit2D/src/VectorHit.cc:1:
/cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_2_X_2020-09-03-1100/src/DataFormats/TrackingRecHit/interface/TrackingRecHit.h:105:40: note: 'virtual std::vector<TrackingRecHit*> TrackingRecHit::recHits()' virtual std::vector<TrackingRecHit*> recHits() = 0;
Outdated
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.
is it really possible to set these values while maintaining all other invariants meaningful?
If not, these modifiers should not be here, use a constructor
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.
I agree, these function don't rally make much sense, and are not used. Removed.
Outdated
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.
What's the reason to also define VectorHitCollectionNew, considering that these are the same?
confusing
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.
Doesn't make sense to me either, I removed VectorHitCollectionNew
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||
| #ifndef DataFormats_TrackerRecHit2D_VectorHit2D_h | ||||||
| #define DataFormats_TrackerRecHit2D_VectorHit2D_h | ||||||
|
|
||||||
JanFSchulte marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| #include "DataFormats/GeometryVector/interface/LocalVector.h" | ||||||
| #include "DataFormats/GeometryVector/interface/LocalPoint.h" | ||||||
| #include "DataFormats/CLHEP/interface/AlgebraicObjects.h" | ||||||
| #include "DataFormats/GeometrySurface/interface/LocalError.h" | ||||||
|
|
||||||
| //Stores geometric information about VectorHits, their convariance matrix and chi2 of the compatability of the two hits | ||||||
|
|
||||||
| class VectorHit2D { | ||||||
| public: | ||||||
| VectorHit2D() : thePosition(), theDirection(), theCovMatrix(), theChi2(), theDimension(2) {} | ||||||
| VectorHit2D(const LocalPoint& pos, const LocalVector& dir, const AlgebraicSymMatrix22& covMatrix, const double& Chi2) | ||||||
|
||||||
| VectorHit2D(const LocalPoint& pos, const LocalVector& dir, const AlgebraicSymMatrix22& covMatrix, const double& Chi2) | |
| VectorHit2D(const LocalPoint& pos, const LocalVector& dir, const AlgebraicSymMatrix22& covMatrix, const double chi2) |
start local variables and data members with lower case letter. Capitalization is reserved for class names.
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.
Fixed.
Outdated
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.
there are no virtual methods, why a virtual destructor? remove (use compiler default given that this is empty)
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.
Replaced with default destructor.
Outdated
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.
what's the motivation to return pointers? const &
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.
Not sure why pointers where returned, does not seem necessary. Changed.
Outdated
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 just a const
| int theDimension; | |
| static constexpr int theDimension = 2; |
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.
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,10 +27,9 @@ void BaseTrackerRecHit::check() const { | |||||||||
| #endif | ||||||||||
|
|
||||||||||
| bool BaseTrackerRecHit::hasPositionAndError() const { | ||||||||||
| //if det is present pose&err are available as well. | ||||||||||
| // //if det() is not present (null) the hit ihas been read from file and not updated | ||||||||||
|
||||||||||
| //if det is present pose&err are available as well. | |
| // //if det() is not present (null) the hit ihas been read from file and not updated | |
| //if det is present pos&err are available as well. | |
| // //if det() is not present (null) the hit has been read from file and not updated |
typos
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.
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.
why not add both clusters already here/now?
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.
I'm not entirely sure what you are proposing here?
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 was my attempt to read the meaning of
//FIXME:: this is just temporary solution for phase2,,compared to SiStripMatchedRecHit2D