refactor(gx2f): put parts into separate compile units #3946
Conversation
WalkthroughSignificant changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
Core/src/TrackFitting/GlobalChiSquareFitter.cpp (2)
60-60: Typo in comment, there is.On line 60, "First, w try to invert..." should be "First, we try to invert..."
71-71: Assist with TODO, I can.The comment "// TODO make dimsExtendedParams template with unrolling" indicates a potential improvement. Offer assistance, I do.
Help you implement this templating, would you like? Open a GitHub issue, shall I?
Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpp (1)
364-380: Detailed documentation, add you should.For
addMeasurementToGx2fSumsBackend, parameter descriptions in the comment block are missing. Consistency in documentation, maintain we must.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpp(6 hunks)Core/include/Acts/Utilities/AlgebraHelpers.hpp(1 hunks)Core/src/TrackFitting/GlobalChiSquareFitter.cpp(1 hunks)
🔇 Additional comments (4)
Core/src/TrackFitting/GlobalChiSquareFitter.cpp (2)
54-140: Well-structured, your new function is.
The addition of addMeasurementToGx2fSumsBackend enhances modularity and clarity in the code by encapsulating measurement addition logic.
142-146: Clean separation of concerns, achieve you have.
The new function computeGx2fDeltaParams neatly handles delta parameter computation, enhancing code readability.
Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpp (1)
Line range hint 396-418: Verify parameters passed correctly, you must.
In addMeasurementToGx2fSums, ensure that all necessary parameters are accurately forwarded to addMeasurementToGx2fSumsBackend.
Core/include/Acts/Utilities/AlgebraHelpers.hpp (1)
199-199: Support for dynamic matrices, added you have.
With rows == -1, safeInverse now handles dynamic-sized matrices. Verify this extension does not introduce unexpected behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
Core/src/TrackFitting/GlobalChiSquareFitter.cpp (4)
71-71: Hmmmm, template this we must, young padawan.A TODO comment about making
dimsExtendedParamstemplate with unrolling, I see. Wise decision this would be, as compile-time dimensions better than runtime dimensions they are. Memory usage and performance, improve this would.Help you implement this templated version, shall I? Create a GitHub issue to track this task, I can.
72-73: Careful with dynamic allocation at runtime, you must be.Zero initialization of large dynamic matrix, I see. Consider reserving capacity beforehand, we should, to prevent multiple reallocations.
- Eigen::MatrixXd extendedJacobian = - Eigen::MatrixXd::Zero(eBoundSize, extendedSystem.nDims()); + Eigen::MatrixXd extendedJacobian(eBoundSize, extendedSystem.nDims()); + extendedJacobian.setZero();
114-139: Verbose logging, impact performance it may.Extensive matrix operations in debug logging, I observe. In production environment, overhead this may cause. Consider guarding expensive operations with debug level check.
- ACTS_VERBOSE( + if (logger.level() <= Acts::Logging::VERBOSE) { + ACTS_VERBOSE( "Contributions in addMeasurementToGx2fSums:\n" // ... existing logging ... - ); + ); + }
54-146: Well structured the code is, hmmmm.Successfully separated the heavy matrix operations into distinct compile units, you have. Clean and maintainable, the code has become. Memory impact during compilation, reduced it shall be.
Consider further optimization through:
- Template specialization for common dimensions
- Pre-allocation of matrices in performance-critical paths
- Compile-time constant expressions where possible
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Core/src/TrackFitting/GlobalChiSquareFitter.cpp(1 hunks)
🔇 Additional comments (1)
Core/src/TrackFitting/GlobalChiSquareFitter.cpp (1)
142-146: 🛠️ Refactor suggestion
Handle matrix singularity, we must.
QR decomposition, reliable it usually is. But prepare for failure, we should. Check rank and condition number, wise it would be.
Eigen::VectorXd Acts::Experimental::computeGx2fDeltaParams(
const Acts::Experimental::Gx2fSystem& extendedSystem) {
- return extendedSystem.aMatrix().colPivHouseholderQr().solve(
- extendedSystem.bVector());
+ auto solver = extendedSystem.aMatrix().colPivHouseholderQr();
+ if (solver.rank() < extendedSystem.aMatrix().rows()) {
+ throw std::runtime_error("Singular matrix in chi-square fitting detected");
+ }
+ return solver.solve(extendedSystem.bVector());
}
|
One remaining issue with the formatting, aside from this looks good to merge. |
|
acts-project-service
added
the
Fails Athena tests
This reduces the memory impact of the GX2F during compilation by:
Impact
main:
after addMeasurementToGx2fSumsBackend:
after computeGx2fDeltaParams:
Other discussion on the topic
Performance of dynamic matrix
This still needs investigation. Maybe we can template the functions again on
kMeasDimsUntemplate temporary track
This was the first attempt, by using
VectorMultiTrajectoryfor the internal tracks. This fails in Athena, because there theMutable...is used. This comes in with the calibrator. Currently, it might work with 2 calibrators (one for the fitting Actor and one for the final Actor). Better solution would be to have type-erasure, which might come in the future.Summary by CodeRabbit
New Features
Improvements