-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[Geom] Fix setting precision for matrix values in TGDMLWrite::CreateM… #20345
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
base: master
Are you sure you want to change the base?
[Geom] Fix setting precision for matrix values in TGDMLWrite::CreateM… #20345
Conversation
|
Thanks! I started the tests waiting for @agheata to review. |
Test Results 21 files 21 suites 3d 19h 45m 49s ⏱️ For more details on these failures, see this check. Results for commit b3788ce. ♻️ This comment has been updated with latest results. |
Hi! I've added a test case for the precision fix in commit e3ea518. The test verifies that matrix values in GDML exports use the configured fFltPrecision setting instead of the default C++ ostream precision (6 digits). Test details:
The test follows the existing pattern used in the geo test suite and integrates with the CMake build system. |
|
This works as well in case you like this more than #20343 |
|
Hi! I've added a test case for the precision fix in commit e3ea518. The test verifies that matrix values in GDML exports use the configured fFltPrecision setting instead of the default C++ ostream precision (6 digits). Test details:
The test follows the existing pattern used in the geo test suite and integrates with the CMake build system. I've also applied clang-format to fix the code style issues. |
agheata
left a comment
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.
Thanks @ahmedbilal9 for catching this! The fix is OK, but not the unit test. The method actually does not apply (in TGeo) to rotation matrices but rather to the more general "matrix" GDML tag. ROOT only reads that tag with TGDMLParse if the GDML is produced externally and contains it; then, it stores the data temporarily in TGeoManager in case it needs to be exported as GDML. So your test does not find the "matrix" tag in the exported file, since ROOT generates "rotation" instead. Besides that, the feature is small and not likely to change after fixing, so I would prefer not adding a unit test anyway. So, could you either rebase your branch and keep a single commit with the fix, or if you prefer make a new PR with that commit, closing the 2 PR's?
…atrixN Fixes root-project#20342 Apply fFltPrecision setting to matrix value exports instead of using default C++ ostream precision (6 digits).
fc759a5 to
b3788ce
Compare
I've rebased the PR to a single commit and removed the test as requested. |
agheata
left a comment
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.
Thanks @ahmedbilal9 for the fix!
Hi @agheata, Thank you for approving the fix! I see some CI tests are failing, but they appear to be unrelated infrastructure issues (mac-beta ARM64, ubuntu2510). The core test suite shows 3,737 passing tests. Should I do anything, or will you merge it as-is? Thanks! |
Fixes #20342
Changes or fixes:
When exporting GDML files, matrix values were being written with the default C++ ostream precision (6 significant digits) instead of using the configured fFltPrecision setting. This caused loss of precision in geometry data.
Applied the same precision formatting pattern used in CreateConstantN to ensure consistent precision across all GDML exports.
Checklist: