feat: add canonical unit vocabulary via mp-units#15
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an mp-units-based physics unit system with canonical quantity aliases and vector conversions, plus typed accessors over persisted SHiP PODs. It adds the ChangesSHiP Units and Quantity Views
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ prek hooks passed |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
include/SHiP/QuantityView.hpp (1)
17-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a template-based approach to reduce repetitive getter/setter boilerplate.
Each of the four types follows the same pattern: scalar getters return
p.field * units::U, scalar setters dop.field = v.numerical_value_in(units::U), vector getters usevecOf<T>(p.field), vector setters useraw(v). A template macro or CRTP-based approach could eliminate ~40 lines of near-identical code. However, the current explicit form is readable and debuggable, so this is purely optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/SHiP/QuantityView.hpp` around lines 17 - 149, The getter/setter blocks for MCParticle, SimHit, SimParticle, and RecParticle are repeated boilerplate. Refactor the repeated patterns in ship::view into a template or CRTP-based helper so scalar fields use the same unit conversion logic and vector fields reuse the same vecOf/raw handling, while preserving the existing explicit API names like vertex, momentum, energy, time, and the corresponding set* functions. Keep the behavior unchanged and only centralize the shared conversion code to reduce duplication.tests/test_units.cpp (1)
42-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a
SimHitsetter round-trip test.
MCParticlegets a full getter→setter→equality round-trip (lines 65–78), butSimHitonly tests thepathLengthgetter (lines 80–85). Theset*functions inQuantityView.hppforSimHit(setPosition,setMomentum,setEnergyDeposit,setTime,setPathLength) are untested. A round-trip test mirroring theMCParticleone would catch bitwise-conversion bugs in those code paths.✨ Suggested addition
SHiP::SimHit hit; hit.position = {-0.5, 0.0, 12000.0}; + hit.momentum = {0.3, -0.1, 50.0}; + hit.energyDeposit = 0.025; + hit.time = 12.3; hit.pathLength = 0.3; expect( ship::view::pathLength(hit).numerical_value_in(su::mm) == hit.pathLength, "SimHit pathLength view is bitwise"); + + SHiP::SimHit hitCopy; + ship::view::setPosition(hitCopy, ship::view::position(hit)); + ship::view::setMomentum(hitCopy, ship::view::momentum(hit)); + ship::view::setEnergyDeposit(hitCopy, ship::view::energyDeposit(hit)); + ship::view::setTime(hitCopy, ship::view::time(hit)); + ship::view::setPathLength(hitCopy, ship::view::pathLength(hit)); + expect(hitCopy.position == hit.position && hitCopy.momentum == hit.momentum && + hitCopy.energyDeposit == hit.energyDeposit && + hitCopy.time == hit.time && hitCopy.pathLength == hit.pathLength, + "SimHit view round-trip is bitwise");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_units.cpp` around lines 42 - 96, The SimHit quantity-view setters are not covered, so add a getter-to-setter round-trip test for SimHit in main that mirrors the MCParticle check. Use ship::view::position, momentum, energyDeposit, time, and pathLength together with the corresponding ship::view::setPosition, setMomentum, setEnergyDeposit, setTime, and setPathLength helpers, then assert the copied SimHit matches the original bitwise. This will locate the missing coverage in the existing SimHit test block and exercise the setter paths in QuantityView.hpp.cmake/SHiPDataModelConfig.cmake.in (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider specifying version and mode in
find_dependency(mp-units).
CMakeLists.txtrequiresmp-units 2.5 CONFIG REQUIRED, but the config template uses a barefind_dependency(mp-units). A downstream consumer with an oldermp-unitsthat lacks the APIs used byUnits.hppwould get a configure-time failure in the imported targets rather than a clear dependency-resolution error. Adding2.5 CONFIGmakes the contract explicit.♻️ Proposed fix
-find_dependency(mp-units) +find_dependency(mp-units 2.5 CONFIG)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmake/SHiPDataModelConfig.cmake.in` at line 5, The dependency declaration in the config template is too loose: `find_dependency(mp-units)` should match the explicit requirement used by the project. Update the `find_dependency` call in the CMake package config template to request `mp-units` version 2.5 in CONFIG mode, so downstream consumers fail during dependency resolution instead of later in `Units.hpp`-related target usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmake/SHiPDataModelConfig.cmake.in`:
- Line 5: The dependency declaration in the config template is too loose:
`find_dependency(mp-units)` should match the explicit requirement used by the
project. Update the `find_dependency` call in the CMake package config template
to request `mp-units` version 2.5 in CONFIG mode, so downstream consumers fail
during dependency resolution instead of later in `Units.hpp`-related target
usage.
In `@include/SHiP/QuantityView.hpp`:
- Around line 17-149: The getter/setter blocks for MCParticle, SimHit,
SimParticle, and RecParticle are repeated boilerplate. Refactor the repeated
patterns in ship::view into a template or CRTP-based helper so scalar fields use
the same unit conversion logic and vector fields reuse the same vecOf/raw
handling, while preserving the existing explicit API names like vertex,
momentum, energy, time, and the corresponding set* functions. Keep the behavior
unchanged and only centralize the shared conversion code to reduce duplication.
In `@tests/test_units.cpp`:
- Around line 42-96: The SimHit quantity-view setters are not covered, so add a
getter-to-setter round-trip test for SimHit in main that mirrors the MCParticle
check. Use ship::view::position, momentum, energyDeposit, time, and pathLength
together with the corresponding ship::view::setPosition, setMomentum,
setEnergyDeposit, setTime, and setPathLength helpers, then assert the copied
SimHit matches the original bitwise. This will locate the missing coverage in
the existing SimHit test block and exercise the setter paths in
QuantityView.hpp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9d1f163-b248-4bfd-b8fc-dcbc74ae7225
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CMakeLists.txtcmake/SHiPDataModelConfig.cmake.ininclude/SHiP/QuantityView.hppinclude/SHiP/Units.hpppixi.tomltests/CMakeLists.txttests/test_units.cpp
Introduce SHiP/Units.hpp (canonical quantity types: mm, ns, GeV, GeV/c, tesla, with momentum and time-of-flight units derived from the exact speed of light) and SHiP/QuantityView.hpp (free-function views that convert the plain-double data classes to and from quantities), exported as the headers-only SHiP::SHiPUnits target. The persisted structs are unchanged: ROOT cannot stream mp-units quantity members (TStreamerInfo::Build fails and the member is silently zeroed on readback), so quantities stop at the I/O boundary by design. Computation code works in quantity types; conversions to and from storage are bitwise because the storage unit is the quantity unit.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/SHiP/QuantityView.hpp`:
- Around line 68-129: Add round-trip coverage in tests/test_units.cpp for the
SimParticle and RecParticle quantity views, exercising each getter/setter pair
exposed by vertex, endpoint, momentum, energy, and time, plus ipPV and setIpPV
for RecParticle. Verify values written through setters are returned unchanged by
the corresponding accessors, including the unique endpoint and ipPV fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a5b8441-4f85-47d0-9280-6ab4dce34207
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CMakeLists.txtcmake/SHiPDataModelConfig.cmake.ininclude/SHiP/QuantityView.hppinclude/SHiP/Units.hpppixi.tomltests/CMakeLists.txttests/test_units.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- cmake/SHiPDataModelConfig.cmake.in
- pixi.toml
- tests/CMakeLists.txt
- tests/test_units.cpp
- CMakeLists.txt
- include/SHiP/Units.hpp
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Adds
SHiP/Units.hpp(canonical quantity types — mm/ns/GeV/GeV-per-c/tesla, with momentum and time-of-flight units derived from the exact speed of light) andSHiP/QuantityView.hpp(free-function views over the data classes), exported as a headers-onlySHiP::SHiPUnitstarget. Persisted structs and dictionary are untouched: ROOT cannot stream quantity members (verified: TStreamerInfo::Build fails, member silently zeroed on readback), so quantities stop at the I/O boundary and conversions are bitwise.First step of the framework-wide mp-units adoption; aegir bridge headers and the field_service alias re-point follow.
Summary by CodeRabbit
ship::viewaccessors providing unit-typed getters/setters for particle and hit fields while preserving the underlying POD layout.units) validating conversions, round-trips, andship::viewaccessor behavior.