refactor: Refactor the measurement calibrator in the examples #2058
Conversation
gagnonlg
added
Improvement
Codecov Report
@@ Coverage Diff @@
## main #2058 +/- ##
==========================================
+ Coverage 49.37% 49.73% +0.36%
==========================================
Files 427 426 -1
Lines 24779 24221 -558
Branches 11430 11006 -424
==========================================
- Hits 12234 12047 -187
+ Misses 4484 4434 -50
+ Partials 8061 7740 -321 see 7 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
📊 Physics performance monitoring for 052ab73Full report VertexingVertexing vs. muIVF seededIVF truth_smearedIVF truth_estimatedIVF orthogonalSeedingSeeding seededSeeding truth_estimatedSeeding orthogonalCKFCKF seededCKF truth_smearedCKF truth_estimatedCKF orthogonalAmbiguity resolutionseededTruth tracking (Kalman Filter)Truth trackingTruth tracking (GSF)Truth tracking |
gagnonlg
force-pushed
the
calibrator-refactor
branch
from
cf868ae to
b35faec
Compare
|
I'm aware of the segfault in some of the tests, I'm investigating... |
gagnonlg
force-pushed
the
calibrator-refactor
branch
from
c4cff6d to
a1093e2
Compare
There was a problem hiding this comment.
Looks good to me, just a few comments
-
The code is under
Examples/Algorithms/Calibration, but there is noCalibrationAlgorithm. Would it be an option to put this code underExamples/Framework/EventDataor something like that? -
I currently find the naming of the classes not optimal, it took me quite a while to understand. Maybe
PairedMeasurementCalibrator->MeasurementCallibratorInterfaceto indicate that this provides the interface to the ACTS core library... -
So far this is not generic enough to work with my refitting-calibrator, as this is not measurement based (It defines an own source-link type, which internally already has all information, so it does not need any container or something like that). But from my perspective this is not a problem.
|
@benjaminhuth Thanks for the feedback! I will change the location & naming of the sources as you suggest. For the refitting calibrator: okay then it makes sense to keep this separate! Thanks again, |
To prepare for implementation of actual calibration methods, made the following changes: + Renamed MeasurementCalibrator to PassThroughCalibrator + Created an abstract base calss MeasurementCalibrator with a calibrate method which takes a measurement container as input + Created a PairedMeasurementCalibrator class that binds a calibrator to a MeasurementContainer, to pass to the fitter
gagnonlg
force-pushed
the
calibrator-refactor
branch
from
3498fe7 to
052ab73
Compare
|
@benjaminhuth I implemented your suggestion. For the renaming, it occured to me that the PairedMeasurementCalibrator is an instance of the adapter pattern, and thus I renamed it to MeasurementCalibratorAdapter. let me know if you have more feedback :-) |
| py::class_<MeasurementCalibrator, std::shared_ptr<MeasurementCalibrator>>( | ||
| mex, "MeasurementCalibrator"); | ||
|
|
||
| mex.def("makePassThroughCalibrator", |
There was a problem hiding this comment.
I know this is merged already, but I there a reason not to just expose the inheritance relationship and then have Python just construct the object directly instead of the factory?
There was a problem hiding this comment.
No real reason I guess, I just don't know what I'm doing on the python side ;-) putting this on my list
I'm currently working on implementing a few different measurement/cluster calibration methods for a bona fide calibration study. The easiest way to run such a study would be to have the option to swap the calibrators on the python side so one does not need to either recompile everytime or duplicate the TrackFitting example for each different methods.
Towards that end, I propose the following changes:
MeasurementCalibratortoPassThroughCalibrator;MeasurementCalibratorwith a calibrate method which takes a measurement container as input;PairedMeasurementCalibratorclass that binds a calibrator to aMeasurementContainer, to pass to the actual fitter;execute()loop.#1940 introduced a alternate path using a RefittingCalibrator instead of a MeasurementCalibrator. I did not touch this code since I'm not sure if it makes sense to unify it with what I have planned -- any thoughts @benjaminhuth ? I propose to leave this as-is for the time being and re-assess once I push actual calibrators (shortly, hopefully).