refactor!: material: double for pathCorrection
#3920
Conversation
WalkthroughChanges made to the Changes
Poem
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: 1
🧹 Outside diff range and nitpick comments (3)
Core/src/Material/AccumulatedMaterialSlab.cpp (2)
Line range hint
26-35: Consistency in precision types, maintain we must.Mixed use of float and double in calculations, I sense. For variance calculation, float you use, but for weights, double you choose. Harmonize these types, we should.
- float variance = ((1 / m_trackAverage.material().X0()) - + double variance = ((1 / m_trackAverage.material().X0()) - (1 / slabReference.material().X0())) * ((1 / m_trackAverage.material().X0()) - (1 / slabReference.material().X0()));
Line range hint
67-71: Return type alignment, crucial it is.Hmmmm... Return float from method that uses double internally, unwise it is. Precision loss at boundaries, this may cause.
-std::pair<float, unsigned int> Acts::AccumulatedMaterialSlab::totalVariance() +std::pair<double, unsigned int> Acts::AccumulatedMaterialSlab::totalVariance() const { return {m_totalVariance, m_totalCount}; }Core/include/Acts/Material/AccumulatedMaterialSlab.hpp (1)
49-49: Consider float to double for variance calculations as well, you should.Notice I do, that while pathCorrection now uses double precision, m_totalVariance still uses float. For consistency in precision throughout calculations, suggest changing m_totalVariance to double as well, I do.
Apply this change, you should:
/// Averaged properties over multiple tracks. MaterialSlab m_totalAverage; /// Averaged variance over multiple tracks. - float m_totalVariance = 0.0; + double m_totalVariance = 0.0;And update the return type in totalVariance():
- std::pair<float, unsigned int> totalVariance() const; + std::pair<double, unsigned int> totalVariance() const;Also applies to: 94-94
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Core/include/Acts/Material/AccumulatedMaterialSlab.hpp(1 hunks)Core/src/Material/AccumulatedMaterialSlab.cpp(1 hunks)
🔇 Additional comments (2)
Core/include/Acts/Material/AccumulatedMaterialSlab.hpp (2)
49-49: Approve the path correction parameter type change, I do!
Wise decision it is, to use double precision for pathCorrection. In physics calculations, more precise we must be! Hmmmm.
49-49: Verify impact on dependent code, we must.
Changed parameter type may affect code that calls this method. Check we must, that all callers handle double precision correctly.
✅ Verification successful
Hmm, safe the change is. Verified our concerns are.
Through the Force, searched the codebase I have. Found many calls to accumulate, but all pass MaterialSlab as first parameter they do. The second parameter pathCorrection, optional it remains with default value of 1.
In AccumulatedMaterialSlab.hpp, changed parameter type affects not the interface - double precision already used it was:
void accumulate(MaterialSlab slabAlongTrack, double pathCorrection = 1.);All callers throughout codebase, proper double precision they use:
- In
AccumulatedSurfaceMaterial.cpp, correct doublepathCorrectionpassed it is - In test files like
AccumulatedMaterialSlabTests.cpp, proper usage shown it is - In material mapping code, correctly handled the types are
Clear it is, that change breaks nothing. Safe to proceed, we are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to accumulate method
rg -A 2 "accumulate\s*\([^)]*\)" --type cpp
Length of output: 17062
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Core/src/Material/AccumulatedMaterialSlab.cpp (2)
Line range hint
67-70: Return type, change it we must.Still returning float for variance, but calculations in double we do. Consistency in the force, maintain we shall.
Apply this change:
-std::pair<float, unsigned int> Acts::AccumulatedMaterialSlab::totalVariance() +std::pair<double, unsigned int> Acts::AccumulatedMaterialSlab::totalVariance()
Line range hint
1-70: Path to consistent precision, clear it must be.Throughout the codebase, a systematic approach to floating-point precision, needed it is. Consider these steps:
- Document precision requirements in coding guidelines
- Create migration plan for remaining float usage
- Add static analysis rules to prevent mixing precisions
Strong with the Force this change is, but complete the journey we must.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Core/src/Material/AccumulatedMaterialSlab.cpp(1 hunks)
🔇 Additional comments (3)
Core/src/Material/AccumulatedMaterialSlab.cpp (3)
Line range hint 24-36: Mixed precision calculations, troubling they are.
Float for X0 calculations but double for weights, confusion this brings. Consistent precision throughout the force, maintain we must.
Suggest these changes, I do:
- float variance = ((1 / m_trackAverage.material().X0()) -
+ double variance = ((1. / m_trackAverage.material().X0()) -
(1 / slabReference.material().X0())) *
((1 / m_trackAverage.material().X0()) -
(1 / slabReference.material().X0()));Line range hint 44-61: Wise implementation, this is.
Consistent use of double precision for weights, good practices it follows. Approve this part, I do.
15-15:
Precision loss in the force, I sense.
Converting pathCorrection to float in scaleThickness, defeats the purpose of double precision it does. Maintain consistent precision throughout the calculations, we must.
Apply this change, you should:
- slab.scaleThickness(static_cast<float>(1. / pathCorrection));
+ slab.scaleThickness(1. / pathCorrection);Hmmmm, but first verify MaterialSlab's thickness type, we must.
Also applies to: 18-18
|
|
Summary by CodeRabbit
New Features
pathCorrectionparameter type todouble.Bug Fixes
Refactor
doublefor related computations, enhancing overall accuracy.