Fix: add missing enable_tensor_type for CartesianLeviCivitaTensor and fix Levi-Civita size() #578#579
Conversation
EmilyBourne
left a comment
There was a problem hiding this comment.
This looks great, but I think at least one extra test would be nice. To tests a different ElementType and maybe to show why CartesianLeviCivitaTensor needs to be enabled as a tensor type
e7ee10e to
ab6925c
Compare
e6a2fd2 to
54eabf7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
b3f35eb to
1e86e55
Compare
9bed90a to
b29dfca
Compare
8f0c7ab to
b570382
Compare
… fix size() clang format, recognised, unauthorized character fixed. space correct
b570382 to
0310308
Compare
|
@Quntized Commits are squashed when we merge so it is not important to preserve a clean commit history. By force pushing your changes down to 1 commit all you are doing is:
|
|
@EmilyBourne Thanks for the clarification! I didn't realize commits are squashed on merge — I'll just push new commits going forward instead of force-pushing. Sorry for the extra CI runs. |
|
@tpadioleau @AbdelhadiKara could someone check that this doesn't cause any problems on GPU before I merge please? My WiFi isn't stable enough to use the vpn |
yes, |
Co-authored-by: Thomas Padioleau <thomas.padioleau@cea.fr>
Co-authored-by: Thomas Padioleau <thomas.padioleau@cea.fr>
|
@EmilyBourne, The tests pass on the V100 architecture, only the simulations related to XVx geometry fail, it is not related to this MR I think. (python tkinter issues) |
…untized/gyselalibxx into fix/static-tensors-improvement
|
@tpadioleau fixed the changes that you suggested and also added in CHANGELOG.md. Let me know if everything looks alright. |
EmilyBourne
left a comment
There was a problem hiding this comment.
Thomas is on holiday. But I agree that his comments have been addressed. I confirm that the code runs and compiles without warnings on GPU
… fix Levi-Civita size() #578 (#579) `src/coord_transformations/static_tensors.hpp`: 1. **Bug fix**: Add missing `enable_tensor_type` specialization for `CartesianLeviCivitaTensor`. 2. **Bug fix**: Fix `size()` in both `CartesianLeviCivitaTensor` and `LeviCivitaTensor`. The previous implementation returned `rank() * rank()` for just rank = 2; 3. **Improvement**: Use `ddc::type_seq_element_t<dim, index_set>` for `IdentityTensor::vector_index_set_t`, 4. **Improvement**: Use `ElementType` instead of hardcoded `double` as the return type of `ddcHelper::get` overloads for special tensors. 5. **Doc fix**: Correct `ddcHelper::get` doc comments "modifiable reference" when the functions return by value. --------- Co-authored-by: Thomas Padioleau <thomas.padioleau@cea.fr>
src/coord_transformations/static_tensors.hpp:Bug fix: Add missing
enable_tensor_typespecialization forCartesianLeviCivitaTensor.Bug fix: Fix
size()in bothCartesianLeviCivitaTensorandLeviCivitaTensor. The previous implementation returnedrank() * rank()for just rank = 2;
Improvement: Use
ddc::type_seq_element_t<dim, index_set>forIdentityTensor::vector_index_set_t,Improvement: Use
ElementTypeinstead of hardcodeddoubleas thereturn type of
ddcHelper::getoverloads for special tensors.Doc fix: Correct
ddcHelper::getdoc comments"modifiable reference" when the functions return by value.
Please complete the checklist to ensure that all tasks are completed before marking your pull request as ready for review.
All Submissions
New Feature Submissions
Changes to Existing Features
Changes to the CI