refactor!: fatras: double for globalAngle
#3922
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: 0
🧹 Outside diff range and nitpick comments (2)
Fatras/src/Physics/NuclearInteraction/NuclearInteraction.cpp (1)
141-144: Good changes these are, but document them we should!Consistent with parameter changes, the local variables now use double precision, they do. Modern C++ style in return statement, I sense. Yet, a comment explaining the units of angles (radians or degrees), helpful it would be.
Add this comment above the return statement:
+ // Returns {phi, theta} in radians in the global coordinate system return {phi, theta};Fatras/include/ActsFatras/Physics/NuclearInteraction/NuclearInteraction.hpp (1)
486-486: Optimize the sampling retry logic, we should.Wise the comment correction is, but improve the retry logic we can. Early exit when trials exceed limit, more efficient it would be.
Consider this improvement:
- if (trials++ % nMatchingTrials == 0) { + if (++trials > nMatchingTrialsTotal) { + return std::nullopt; + } + if (trials % nMatchingTrials == 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Fatras/include/ActsFatras/Physics/NuclearInteraction/NuclearInteraction.hpp(3 hunks)Fatras/src/Physics/NuclearInteraction/NuclearInteraction.cpp(2 hunks)
🔇 Additional comments (4)
Fatras/src/Physics/NuclearInteraction/NuclearInteraction.cpp (2)
116-117: Approve these changes, I do! Hmmmm.
Wise decision it is, to use double precision for angular calculations. More accurate the physics simulations shall be, yes!
Line range hint 116-144: Verify precision consistency in related code paths, we must!
Through the Force, sense other places where precision matters, I do. Check we must, that no precision is lost when these values are used.
Fatras/include/ActsFatras/Physics/NuclearInteraction/NuclearInteraction.hpp (2)
516-522: Clear and elegant, this code has become. Approve it, I do!
Strong with the Force, these changes are:
- Structured binding for better readability
- Modern constant usage with std::numbers::pi
- Clear direction calculation
294-295: Approve the parameter type change, I do! Hmmmm.
Wise decision it is, to use double precision for angle calculations. Better accuracy this brings to the Force of nuclear interactions.
Verify the impact of this change throughout the codebase, we must:
✅ Verification successful
Consistent with the Force, this change is!
Observed have I, that all callers of globalAngle already use double precision, they do! No conflicts in the codebase exist. While other angle calculations still use float in different parts of the codebase, they do, independent systems they are, affecting not our nuclear interactions, hmm.
The implementation and its usage:
- In
NuclearInteraction.cpp, double precision it uses - In
NuclearInteraction.hpp, structured binding with double precision it employs - All callers, aligned with double precision they are
Safe this change is, as isolated within the nuclear interaction system it remains. Other float usages in the codebase, different concerns they address, hmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining float usage in angle calculations
rg -g '!*.{md,txt}' '\b(float|Float)\b.*\b(angle|theta|phi)\b'
Length of output: 5051
Script:
#!/bin/bash
# Search for usage of globalAngle function to check if callers expect float or double
rg -A 3 "globalAngle\("
# Check the implementation of globalAngle to understand the precision requirements
ast-grep --pattern 'globalAngle($$$) {
$$$
}'
Length of output: 1623
|
|
Note, I think that this code is unmonitored and unused. I also believe this is not breaking |
WE could remove this then? |
|
removal is fine for me, I will try :) |
|
Closed in favour of a full removal: |
Summary by CodeRabbit
New Features
globalAnglemethod.sampleContinuousValuesmethod for better robustness.Bug Fixes
sampleKinematicsmethod for clarity.Refactor
globalAnglemethod for improved readability.