-
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 110 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,156 @@ | ||||||
| #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() { setType(bad); } | ||||||
|
|
||||||
| //VectorHit(const VectorHit& vh); | ||||||
|
||||||
|
|
||||||
| VectorHit(const GeomDet& idet, | ||||||
| const LocalPoint& posInner, | ||||||
| const LocalVector& dir, | ||||||
| const AlgebraicSymMatrix44 covMatrix, | ||||||
|
||||||
| const AlgebraicSymMatrix44 covMatrix, | |
| const AlgebraicSymMatrix44& covMatrix, |
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.
JanFSchulte marked this conversation as resolved.
Show resolved
Hide resolved
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.
| AlgebraicSymMatrix44 covMatrix() const; | |
| const AlgebraicSymMatrix44& covMatrix() const; |
the current use case is visible, but somewhat negligible
https://slava77sk.web.cern.ch/slava77sk/reco/cgi-bin/igprof-navigator/jschulte/igreport_perf_PU200_72ba9db/2356
Still, if someone wants to get a few elements, no need to require a full copy.
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.
JanFSchulte marked this conversation as resolved.
Show resolved
Hide resolved
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 this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant
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.
Accidental leftover, removed.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,36 @@ | ||||||||||||||||||
| #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() {} | ||||||||||||||||||
| VectorHit2D(const LocalPoint& pos, const LocalVector& dir, const AlgebraicSymMatrix22& covMatrix, const double& chi2) | ||||||||||||||||||
| : thePosition(pos), | ||||||||||||||||||
| theDirection(dir), | ||||||||||||||||||
| theCovMatrix(covMatrix), | ||||||||||||||||||
| theLocalError(theCovMatrix[0][0], theCovMatrix[0][1], theCovMatrix[1][1]), | ||||||||||||||||||
| theChi2(chi2){}; | ||||||||||||||||||
|
||||||||||||||||||
| theChi2(chi2){}; | |
| theChi2(chi2){} |
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.
| const LocalPoint localPosition() const { return thePosition; } | |
| const LocalVector localDirection() const { return theDirection; } | |
| const LocalError localDirectionError() const { return theLocalError; } | |
| const AlgebraicSymMatrix22 covMatrix() const { return theCovMatrix; } | |
| const LocalPoint& localPosition() const { return thePosition; } | |
| const LocalVector& localDirection() const { return theDirection; } | |
| const LocalError& localDirectionError() const { return theLocalError; } | |
| const AlgebraicSymMatrix22& covMatrix() const { return 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.
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