Skip to content
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

Separate quaternion algebra from particle rotation #3164

Merged
merged 12 commits into from
Oct 15, 2019

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Sep 12, 2019

Description of changes:

Use Vector4d arithmetic and std::tuple, remove unused error codes,
simplify the Cython interface by enabling Vector4d indexing.
@jngrad jngrad added the Core label Sep 12, 2019
@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #3164 into python will decrease coverage by <1%.
The diff coverage is 99%.

Impacted file tree graph

@@          Coverage Diff           @@
##           python   #3164   +/-   ##
======================================
- Coverage      85%     85%   -1%     
======================================
  Files         528     530    +2     
  Lines       25790   25777   -13     
======================================
- Hits        22148   22136   -12     
+ Misses       3642    3641    -1
Impacted Files Coverage Δ
src/core/virtual_sites/VirtualSitesRelative.hpp 100% <ø> (ø) ⬆️
src/utils/tests/quaternion_test.cpp 100% <100%> (ø)
src/utils/include/utils/math/quaternion.hpp 100% <100%> (ø)
src/core/rotation.cpp 97% <100%> (-1%) ⬇️
src/core/collision.cpp 80% <100%> (ø) ⬆️
src/core/particle_data.hpp 97% <100%> (-1%) ⬇️
src/core/particle_data.cpp 96% <100%> (-1%) ⬇️
src/core/rotation.hpp 100% <100%> (ø) ⬆️
src/core/virtual_sites/VirtualSitesRelative.cpp 86% <100%> (ø) ⬆️
src/core/virtual_sites.cpp 85% <97%> (-3%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b7b357...4b92283. Read the comment docs.

@RudolfWeeber
Copy link
Contributor

I'd suggest deferring this until after 4.1. The freeze is already a few days overdue and I'd like to focus on release-relevant issues, right now.

Copy link
Contributor

@fweik fweik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move quaternion.hpp to utils and add unit tests for the three functions in there.

src/core/quaternion.hpp Outdated Show resolved Hide resolved
src/core/quaternion.hpp Outdated Show resolved Hide resolved
src/core/quaternion.hpp Outdated Show resolved Hide resolved
src/core/quaternion.hpp Outdated Show resolved Hide resolved
src/core/quaternion.hpp Outdated Show resolved Hide resolved
@jngrad
Copy link
Member Author

jngrad commented Sep 13, 2019

@fweik I personally wouldn't move that file to utils because it's not re-usable. The corner case with +/-pi/2 shows this quaternion implementation only works because the calling sites were tailored for it. Writing a C++ unit test would not really make sense here, because only the final result of the rotation is of interest (already covered by Python tests), and it would make maintenance harder (we once had a C++ tabulated bond with the wrong sign, and a test that made sure the result had the wrong sign, because the python logic was tailored for it). Coming up with a unit test is also annoying because the quaternion formalism is not standard, and the fact that a unit test was hard-coded inside a calling site to check the rotation inversion implementation is correct every time the function is called is odd.

@fweik
Copy link
Contributor

fweik commented Sep 13, 2019

Please add the tests.

@fweik
Copy link
Contributor

fweik commented Sep 13, 2019

Testing the multiplication is easy, just check the multiplication table. For the conversion functions you should check that they are inverse to each other. It's sufficient that they encode the direction, not how. If anything, when there is client code that depends on a specific behavior of the function, especially it is non-standard, that gives more reason to add a test, so that behavior is kept. So you should also add a test for the sign convention.
Also the file should be moved, it's generic math and not related to MD or the espressos core, so it is a better fit for utils.

The C++ standard specifically excludes std::sqrt from the list of
functions in cmath that are eligible for constexpr. Although some
compilers made std::sqrt a constexpr function anyway, it's not the
case of AppleClang 9.0.
@jngrad jngrad added this to the Espresso 5 milestone Sep 20, 2019
@jngrad jngrad requested a review from fweik October 12, 2019 19:06
@jngrad
Copy link
Member Author

jngrad commented Oct 15, 2019

bors r=fweik

bors bot added a commit that referenced this pull request Oct 15, 2019
3164: Separate quaternion algebra from particle rotation r=fweik a=jngrad

Description of changes:
- extract quaternion algebra from the `particle_data.hpp` and `rotation.hpp` files
   - removes code duplication caused by a (now resolved) circular dependency (see #3157)
   - makes it possible to replace the quaternion code by a dedicated library, e.g. [boost:qvm::quat](https://www.boost.org/doc/libs/1_68_0/libs/qvm/doc/index.html) or [boost::math::quaternion](https://www.boost.org/doc/libs/1_62_0/libs/math/doc/html/quaternions.html) in the core (see #2289), [rowan](https://rowan.readthedocs.io/en/latest/) in the interface (see #2964)
- simplify code around call sites using Vector4d arithmetic, `std::tuple`, Particle references
- documentation cleanup

3245: Fix thermostat and integrator checkpointing r=KaiSzuttor a=jngrad

The checkpointing mechanism silently broke in 4.1 for the SD and NPT integrators and LB and NPT thermostats. This was fixed, and now all integrators and thermostats checkpoints are tested in CI.

Co-authored-by: Jean-Noël Grad <[email protected]>
Co-authored-by: Kai Szuttor <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 15, 2019

Build succeeded

@bors bors bot merged commit 4b92283 into espressomd:python Oct 15, 2019
@jngrad jngrad modified the milestones: Espresso 5, Espresso 4.2 Nov 20, 2019
@jngrad jngrad deleted the extract-quaternions branch January 18, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants