[WIP] KF v0 review #10
There are no files selected for viewing
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||
| import FWCore.ParameterSet.Config as cms | ||||||
|
|
||||||
| # This modifier is for injecting KF-based iterations in TICL. | ||||||
|
|
||||||
| kf = cms.Modifier() | ||||||
|
Owner
Author
There was a problem hiding this comment. I suggest changing the name of the Modifier to make it more explicit, something along the lines of:
Suggested change
Owner
Author
There was a problem hiding this comment. Remember when you open the PR to include a line with a recipe to test the Modifier for the reviewers. There was a problem hiding this comment. Ok sounds good. But separate from the code right? So it's gonna be in the comment section along the lines of "Test modifier on xxxxx.x" right? |
||||||
Empty file.
|
Owner
Author
There was a problem hiding this comment. The question will come on "Do you really need a new hit or you can re-use some that is already in DafaFormats/TrackingRecHit?". Be prepared to reply to this. Moreover, there will be the question of why it needs another folder and it cannot just be under 'DataFormats/HGCRecHit/' There was a problem hiding this comment. So the reason why we can't use the TrackingRecHit directly is that it contains several pure virtual functions meaning that it can only be used as part of a derived class. I think the same goes for the rest of the objects found in DataFormats/TrackingRecHit. What I could try is to use maybe TrackerRecHit objects in https://github.com/cms-sw/cmssw/tree/master/DataFormats/TrackerRecHit2D for example. That I didn't try. As for the folder: Sure I think I can move it to the HGCRecHit folder There was a problem hiding this comment. But come to think of it. The HGCRecHit is based on a different type of RecHit so maybe it does make sense to separate the two?
Owner
Author
There was a problem hiding this comment. Yes, I agree with you. |
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| #ifndef RecoHGCal_HGCTracking_HGCTrackingRecHit_h | ||
| #define RecoHGCal_HGCTracking_HGCTrackingRecHit_h | ||
|
|
||
| /// Wrapper for TrackingRecHits for PatternRecognitionByKF | ||
|
|
||
| #include <cassert> | ||
| #include "DataFormats/TrackingRecHit/interface/RecHit2DLocalPos.h" | ||
|
|
||
| class HGCTrackingRecHit : public RecHit2DLocalPos { | ||
| public: | ||
| HGCTrackingRecHit() {} | ||
| HGCTrackingRecHit(DetId id, const LocalPoint &pos, const LocalError &err, const float energy): | ||
| RecHit2DLocalPos(id), | ||
| pos_(pos), err_(err), energy_(energy) {} | ||
|
|
||
| virtual HGCTrackingRecHit * clone() const override { return new HGCTrackingRecHit(*this); } | ||
| virtual LocalPoint localPosition() const override { return pos_; } | ||
| virtual LocalError localPositionError() const override { return err_; } | ||
| float energy() const { return energy_; } | ||
|
|
||
| protected: | ||
| LocalPoint pos_; | ||
| LocalError err_; | ||
| float energy_; | ||
| }; | ||
|
|
||
| #endif |
Empty file.
Empty file.
|
Owner
Author
There was a problem hiding this comment. Decide:
|
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| // Author: Mark Matthewman - [email protected] | ||
| // Date: 04/2023 | ||
|
|
||
| // Note!!!! | ||
| // KFHit is a temporary class provided for easier handling of the PatternRecognitionByKF. | ||
| // Once the result format for the Algo has been decided, then the KFHit is obsolete and needs to be deleated. | ||
|
|
||
| #ifndef DataFormats_HGCalReco_KFHit_h | ||
| #define DataFormats_HGCalReco_KFHit_h | ||
|
|
||
| #include <array> | ||
| #include <vector> | ||
| #include "DataFormats/Provenance/interface/ProductID.h" | ||
| #include "DataFormats/Math/interface/Vector3D.h" | ||
| #include "TrackingTools/TrajectoryState/interface/TrajectoryStateOnSurface.h" | ||
| #include <Eigen/Core> | ||
|
|
||
| struct KFHit | ||
| { | ||
| explicit KFHit(TrajectoryStateOnSurface tsos, uint32_t id): | ||
| center(tsos.globalPosition()), | ||
| localError(tsos.localError().matrix()), | ||
| cartesianError(tsos.cartesianError().matrix()), | ||
| curvilinearError(tsos.curvilinearError().matrix()), | ||
| xx(tsos.localError().positionError().xx()), | ||
| xy(tsos.localError().positionError().xy()), | ||
| yy(tsos.localError().positionError().yy()), | ||
| charge(tsos.charge()), | ||
| detid(id) | ||
| {} | ||
| KFHit(){} | ||
|
|
||
| GlobalPoint center; | ||
| AlgebraicSymMatrix55 localError; | ||
| AlgebraicSymMatrix66 cartesianError; | ||
| AlgebraicSymMatrix55 curvilinearError; | ||
| float xx; | ||
| float xy; | ||
| float yy; | ||
| int charge; | ||
| uint32_t detid; | ||
| }; | ||
| #endif |
This file contains hidden or 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
This file contains hidden or 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
|
Owner
Author
There was a problem hiding this comment. Also in this case, the question will come on "Do you really need this class or you can re-use some that is already in there?". Be prepared to reply to this. |
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,35 @@ | ||||||
| #ifndef RecoHGCal_HGCTracking_HGCDiskGeomDet_h | ||||||
| #define RecoHGCal_HGCTracking_HGCDiskGeomDet_h | ||||||
|
|
||||||
| // (mmatthew) Wrapper class for GeomDet corresponding to one layer of HGCal. Used by PatternRecognitionByKF. | ||||||
|
Owner
Author
There was a problem hiding this comment.
Suggested change
|
||||||
|
|
||||||
| #include "Geometry/CommonDetUnit/interface/GeomDet.h" | ||||||
|
Owner
Author
There was a problem hiding this comment. Not important, but more correct:
Suggested change
|
||||||
| #include "DataFormats/ForwardDetId/interface/HGCSiliconDetId.h" | ||||||
| #include "DataFormats/ForwardDetId/interface/HGCScintillatorDetId.h" | ||||||
|
Comment on lines
+7
to
+8
Owner
Author
There was a problem hiding this comment. Do you need to include these two? |
||||||
| #include "FWCore/Utilities/interface/Exception.h" | ||||||
|
|
||||||
| class HGCDiskGeomDet : public GeomDet { | ||||||
| public: | ||||||
| HGCDiskGeomDet(int subdet, int zside, int layer, float z, float rmin, float rmax, float radlen, float xi) ; | ||||||
|
|
||||||
| int subdet() const { return subdet_; } | ||||||
| int zside() const { return zside_; } | ||||||
| int layer() const { return layer_; } | ||||||
| float rmin() const { return rmin_; } | ||||||
| float rmax() const { return rmax_; } | ||||||
| bool isSilicon() const { | ||||||
|
Owner
Author
There was a problem hiding this comment. maybe you can add as a comment the file where this enum is taken. |
||||||
| if(subdet_ == 8 || subdet_ == 9){ | ||||||
| return true; | ||||||
| } | ||||||
| else if (subdet_ == 10){ | ||||||
| return false; | ||||||
| } | ||||||
| else throw cms::Exception("LogicError", "Subdetector not defined"); | ||||||
| } | ||||||
|
|
||||||
| protected: | ||||||
| const int subdet_, zside_, layer_; | ||||||
| const float rmin_, rmax_; | ||||||
| }; | ||||||
|
|
||||||
| #endif | ||||||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| #include "Geometry/CommonTopologies/interface/HGCDiskGeomDet.h" | ||
| #include "DataFormats/GeometrySurface/interface/MediumProperties.h" | ||
| #include "DataFormats/GeometrySurface/interface/BoundDisk.h" | ||
|
|
||
| HGCDiskGeomDet::HGCDiskGeomDet(int subdet, int zside, int layer, float z, float rmin, float rmax, float radlen, float xi) : | ||
| GeomDet( Disk::build(Disk::PositionType(0,0,z), Disk::RotationType(), SimpleDiskBounds(rmin, rmax, -20, 20)).get() ), | ||
| subdet_(subdet), zside_(zside), layer_(layer), rmin_(rmin), rmax_(rmax) | ||
| { | ||
| if(subdet == 8 || subdet == 9) setDetId(DetId::Detector(subdet)); | ||
| if (radlen > 0) { | ||
| (const_cast<Plane &>(surface())).setMediumProperties(MediumProperties(radlen,xi)); | ||
| } | ||
| } |
This file contains hidden or 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
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| <flags CXXFLAGS="-g -D=EDM_ML_DEBUG"/> | ||
|
Owner
Author
There was a problem hiding this comment. clean up (maybe keep in debug branch) |
||
| <use name="DataFormats/Candidate"/> | ||
| <use name="DataFormats/HGCalReco"/> | ||
| <use name="DataFormats/VertexReco"/> | ||
| <use name="FWCore/PluginManager"/> | ||
| <use name="PhysicsTools/TensorFlow"/> | ||
| <use name="TrackingTools/TrajectoryState"/> | ||
| <export> | ||
| <lib name="1"/> | ||
| </export> | ||
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
|
|
This file contains hidden or 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
|
|
This file contains hidden or 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
|
|
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,4 +50,4 @@ namespace ticl { | |
| }; | ||
|
|
||
| } // namespace ticl | ||
| #endif | ||
Oops, something went wrong.
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.
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 believe this file needs to be re-inserted.
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.
But this is only needed for my branch and I don't think it should be part of the framework right?