-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support native CLHEP unit system #1085
Conversation
f2cf96c
to
75fcfcd
Compare
Thanks Seth! Looks great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, this is great @sethrj!
Are the tests expected to pass in all unit systems now? I'm still seeing a handful failing with SI and CLHEP (looks like device tests).
@@ -157,6 +164,11 @@ TEST_F(MaterialTest, params) | |||
|
|||
TEST_F(MaterialTest, material_view) | |||
{ | |||
if (CELERITAS_UNITS != CELERITAS_UNITS_CGS) | |||
{ | |||
GTEST_SKIP() << "Test needs updating to include units"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you already updated part of this test... were you planning on updating the rest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got lazy at some point... I just wanted enough to show that the code essentially works, and later (next Christmas?) polish everything to a shine.
Tests are passing now with CLHEP, but I'm still seeing the following failures with SI: 88 - celeritas/Constants (Failed)
96 - celeritas/em/LivermorePE (Failed)
103 - celeritas/em/UrbanMsc (Failed)
111 - celeritas/ext/GeantGeo (Failed)
113 - celeritas/ext/GeantImporter:FourSteelSlabs* (Failed)
115 - celeritas/ext/GeantImporter:OneSteelSphere.* (Failed)
124 - celeritas/field/LinearPropagator (Failed)
136 - celeritas/global/AlongStep:SimpleCmsRZFieldAlongStepTest.msc_rzfield (Failed)
166 - celeritas/phys/Physics (Failed)
186 - celeritas/track/TrackSort (Failed)
208 - app/demo-rasterizer (Failed) |
Ah I never even tested the whole thing with SI (only tested the units/constants). How about I change the title and fix SI in a subsequent PR? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, sure that works. Thanks!
This allows users to configure Celeritas to process "native" values in a different unit system. Having multiple unit systems supported by the code in CI provides a valuable check for correctness of units in physics code, unit tests, and translation layers.
Following on to #1076 as a holiday/vacation project, this fixes several unit system scaling issues in Celeritas and fixes (or disables) issues with missing system of units in the unit tests.
CLHEP::cm
with a variableclhep_length
which translates from CLHEP to the native unit systemThings to note:
Changes split into separate PRs: