fix: Fix combination of material for molar electron density and mean excitation energy
#4079
Conversation
WalkthroughEnhance the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (21)
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 (
|
andiwand
changed the title
molar electron density and mean excitation energy
![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
🧹 Nitpick comments (3)
Core/include/Acts/Material/Material.hpp (2)
132-133: Documentation for new members, improve we should!Missing documentation comments for these new members, I sense. Help future padawans understand their purpose, we must.
Add documentation like this, you should:
+ /// Molar electron density in mol/length³ float m_molarElectronRho = 0.0f; + /// Mean excitation energy in native energy units float m_meanExcitationEnergy = 0.0f;
144-150: Floating-point comparisons, tricky they are!Direct equality comparison for floating-point values, dangerous it can be. Consider epsilon-based comparison for floating-point members, we should.
Consider implementing like this:
- (lhs.m_molarElectronRho == rhs.m_molarElectronRho) && - (lhs.m_meanExcitationEnergy == rhs.m_meanExcitationEnergy); + (std::abs(lhs.m_molarElectronRho - rhs.m_molarElectronRho) < epsilon) && + (std::abs(lhs.m_meanExcitationEnergy - rhs.m_meanExcitationEnergy) < epsilon);Core/src/Material/AverageMaterials.cpp (1)
130-146: Complex calculations, handle with care we must!Exponential averaging for mean excitation energy, complex it is. Consider adding comments explaining the physics behind this approach, we should.
Add explanatory comments like this:
+ // For mean excitation energy, we use geometric averaging when both values are positive + // as it better represents the physics of energy loss in combined materials. + // For zero or negative values, we fall back to arithmetic averaging. float meanExcitationEnergy = 0.f;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Core/include/Acts/Material/Material.hpp(7 hunks)Core/src/Material/AverageMaterials.cpp(2 hunks)Core/src/Material/Material.cpp(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_debug
🔇 Additional comments (4)
Core/src/Material/Material.cpp (3)
30-39: Approve these functions, I do!Well implemented, these constexpr functions are. Efficient at compile-time, they shall be. Clear and focused, their purpose is.
58-60: Careful with precision loss, we must be!From double to float, the conversion goes. In calculations where high precision matters, impact this might have.
Consider if double precision throughout the calculation chain, needed it is.
70-82: Wise implementation, this is!Good use of optional parameters and clear parameter handling, I see. Strong with the Force, this implementation is.
Core/src/Material/AverageMaterials.cpp (1)
35-40: Handle zero thickness wisely, you do!Prevent division by zero, these checks do. Return early pattern, clean it is.
|
There's some conflicts, but also CI failures, can you check @andiwand ? |
github-actions
bot
added
Component - Examples
|
During working on dense propagation I discovered that our material combination does not work for the molar electron density and the mean excitation energy. To fix this I have put these quantities into new variables which are averaged correctly and derived if not provided by the user.
In the future we may want to think about expanding the material param vector to include these quantities.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation