[WIP] KF v0 review #10

ebrondol · 2023-05-04T12:42:20Z

This is a fake pull request to track the review for the Kalman Filter code for HGCAL in CMSSW.

…erBtau Remove dead assignments and simplify HLTmumutkFilter

The medium properties were gained via the neutrinogun. There, the radlen was calculated. The xi was calculated from the radlen and other properties specified in the MediumProperties.h

Updated nextDisk and added HGCDiskGeomDet so that the propagator propagates to the correct z position based on the type of sensor.

measurements function gets the closest measurements to the output of the propagator based on Chi2MeasurementEstimator. HGCTrackingRecHits needed as input for estimator. The local error for the individual sensors is calculated before the event loop. Update step performed using output of propagator and measurements. TODO: Algorithm fails at low energies and at fringe regions.

A segmentation fault occurred due to missing tracks for the seeding region. Now skipping events with missing tracks. The for loop in the nextDisk() function did not account for the case where, in the forward propagation, the track moves from a scintillator tile to a silicon cell. In the context of the for loop this requires 3 iterations instead of two to move to the silicon in the next layer (scintillator, silicon, scintillator, silicon).

…SSW_13_1_0_pre1

ebrondol

Hi @mnmat
Thanks a lot for the work, my comments are mostly for clean-up and to include descriptions or comments in the code to improve readability.

ebrondol · 2023-05-04T12:43:10Z

.gitignore

I believe this file needs to be re-inserted.

But this is only needed for my branch and I don't think it should be part of the framework right?

ebrondol · 2023-05-04T12:44:59Z

Configuration/ProcessModifiers/python/kf_cff.py

+
+# This modifier is for injecting KF-based iterations in TICL.
+
+kf =  cms.Modifier()


I suggest changing the name of the Modifier to make it more explicit, something along the lines of:

Suggested change

kf = cms.Modifier()

hgcal_ticl_kf = cms.Modifier()

Remember when you open the PR to include a line with a recipe to test the Modifier for the reviewers.

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?

ebrondol · 2023-05-04T12:49:35Z

DataFormats/HGCTrackingRecHit/interface/HGCTrackingRecHit.h

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/'

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

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?

Yes, I agree with you.
Let's keep it as it is and if they ask in the review to move it, I would say either we move it in 'DataFormats/TrackingRecHit' or in 'DataFormats/TrackerRecHit2D'. Maybe better the first one because I looked into the classes of the 2D and I don't think there is one that we can simple use for our scope.

ebrondol · 2023-05-04T12:52:39Z

DataFormats/HGCalReco/interface/KFHit.h

Decide:

what to keep in the rechit object

in case you want to keep it for debugging purposes make another branch and clean up this one

ebrondol · 2023-05-04T12:53:30Z

DataFormats/HGCalReco/src/classes.h

 #include <vector>
 #include <array>
 #include "DataFormats/HGCalReco/interface/Trackster.h"
+#include "DataFormats/HGCalReco/interface/KFHit.h"


Clean up if needed.

ebrondol · 2023-05-04T14:35:06Z

RecoHGCal/TICL/plugins/PatternRecognitionbyKF.cc

+    traj_kf.swap(newcands_kf);
+  }
+
+  // Propagator loop used purely for testing purposes


to move to debug branch?

ebrondol · 2023-05-04T14:36:26Z

RecoHGCal/TICL/plugins/PatternRecognitionbyKF.cc

+  }
+
+  FreeTrajectoryState fts = trajectoryStateTransform::outerFreeState(tkx.front(),bfield_.product());
+  std::cout << "Rescale Factor: None" << std::endl;


In this version of the code, the rescaling is not implemented. TBH, I would implement it and keep it to the user in case they want to play with it.

ebrondol · 2023-05-04T14:43:51Z

RecoHGCal/TICL/plugins/PatternRecognitionbyKF.h

@@ -0,0 +1,114 @@
+// Author: Marco Rovere - [email protected]


to be changed

ebrondol · 2023-05-04T14:47:22Z

RecoHGCal/TICL/plugins/PatternRecognitionbyKF.cc

+  }
+
+  // Collect hits with estimate
+  int depth = disk->layer()+1;


I would call it layer if this is indeed what it is. depth can be a bit mysterious

ebrondol · 2023-05-04T14:56:47Z

RecoHGCal/TICL/plugins/PatternRecognitionbyKF.cc

+        if (rho < rmin[layer]) rmin[layer] = rho;
+    }
+
+  int layer = ddd->getLayerOffset(); // FIXME: Can be made less stupid


stupid question: why this and not firstLayer(): https://github.com/cms-sw/cmssw/blob/master/Geometry/HGCalCommonData/interface/HGCalDDDConstants.h#LL70C23-L70C23
?

cmsbuild and others added 15 commits May 25, 2022 16:28

Merge pull request cms-sw#36977 from perrotta/removeDeadCodeInHltrigg…

4eab938

…erBtau Remove dead assignments and simplify HLTmumutkFilter

Added propagator

142e455

Added hardcoded medium properties

101e62a

The medium properties were gained via the neutrinogun. There, the radlen was calculated. The xi was calculated from the radlen and other properties specified in the MediumProperties.h

WIP

7551077

Correct handling of Disks for sensor offset

6495321

Updated nextDisk and added HGCDiskGeomDet so that the propagator propagates to the correct z position based on the type of sensor.

Added RecHit option to TICLLayerTileProducer

7a6a582

Merge branch 'TICL_KF' of https://github.com/mnmat/cmssw into from-CM…

a1dffe6

…SSW_13_1_0_pre1

Move to CMSSW_13_1_0_pre1

02a3f61

Move MB, Fix local error, export detids

1cfbd0a

WIP

e09e955

Refactor

e4fdb65

WIP

26ca108

Merged PatternRecognitionByKF from repository mnmat with cms-merge-topic

c1690a3

ebrondol commented May 4, 2023

View reviewed changes


		# This modifier is for injecting KF-based iterations in TICL.

		kf = cms.Modifier()

		@@ -0,0 +1,114 @@
		// Author: Marco Rovere - [email protected]

[WIP] KF v0 review #10

Are you sure you want to change the base?

[WIP] KF v0 review #10

Uh oh!

Conversation

ebrondol commented May 4, 2023

Uh oh!

ebrondol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants