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

Some alignment of quaternion formalism with the referenced works #2964

Closed
bogdan-tanygin opened this issue Jul 1, 2019 · 12 comments · Fixed by #3392
Closed

Some alignment of quaternion formalism with the referenced works #2964

bogdan-tanygin opened this issue Jul 1, 2019 · 12 comments · Fixed by #3392

Comments

@bogdan-tanygin
Copy link
Contributor

In regards to all the key quaternion related equations in the rotation.cpp... There are references sonnenschein1985 and Omelyan1998 though the code and Martys1999 in the code header and in the documentation. A comparison of the formulae provides a match between these 3 works. For the source code: if one compares each component index of the quaternion and such indices of the torque, omega and other Cartesian (hereinafter, it means 3D space) entities then there is a mismatch with the reference work formulae. Of course, all the formalism works fine now. A quick analysis result: this is just a formal redefinition of the quaternion space with some other alignments: the 4-dim space axes permutation and some inversion. Note, that the Cartesian components are the same. This is why most of formulae looks different. Probably, the real original reference work was different or some other formalism has been used initially. Just one example of practical confusion of this mismatch (there are others as well: with the director, etc):

  • from Es. documentation: For a value of (1,0,0,0), the body and space frames coincide.

  • from Eq. 3 of sonnenschein1985: see the image (the rotation matrix):
    image
    Obviously, the ksi is 0-th element of the quaternion. If we set 1 there (from q=1 0 0 0 value) then we have the rotational matrix diagonal elements: -1 1 -1. The Eq. 3 will require q=0 0 0 1 in this case. Actually, we can see the the Eq. 3 is different compare to the matrix in define_rotation_matrix().

The same for other equations. Nothing serious, but it, probably, leads to some confusions when someone will try to use the quaternions from Es. in a context of other equations and the formalism and simulation methods which are based on the conventions of the Martys1999, other references with such formalism.

Unless, I seriously misunderstood something, please correct me if this is the case... I'm ready to help: either documentation based or the source code adaptations (permute the indices in the Q formulae, maybe align some tests, etc). You can decide, but I suggest the latter solution cause it is really sophisticated to work with the existing quaternion space when trying to develop some tailored extra features of scientific computing... At least for me :-)

@bogdan-tanygin
Copy link
Contributor Author

a bit related discussions: #1048, #1854

@bogdan-tanygin
Copy link
Contributor Author

this can be related to the notation convention options: either 1st or last component of the quaternion is real. However, probably, this does not change my above comments ;-)

@RudolfWeeber
Copy link
Contributor

It is not clear to me, whether this would be a user-visible change. Does this change the numeric values of ParticleHandle.quat representing a specific orientation?

@bogdan-tanygin
Copy link
Contributor Author

bogdan-tanygin commented Sep 12, 2019 via email

@pdebuyl
Copy link
Contributor

pdebuyl commented Sep 12, 2019

Hey, just pitching in :-)

One important issue is whether one can store quaternions in the output data. If so, I would like to support -- if possible -- to conform to the storage scheme of a nice quaternion library in Python, rowan. Their scheme is "scalar i j k" components.

For processing the rotational dynamics via, say, H5MD files, this would make it very convenient :-)

@bogdan-tanygin
Copy link
Contributor Author

in this case, I suppose, that existing quaternion scheme is already fine. Cause, for a value of (1,0,0,0), the body and space frames coincide. I need to check, but I think so. So, only a reference correction (addition), perhaps, is needed. I can do it if needed.

@jngrad
Copy link
Member

jngrad commented Sep 12, 2019

In my opinion, the current formalism is espresso is not satisfactory. The indices do not match those from other libraries like boost::math::quaternion, and operations like rotation inversion and first/second derivative would probably be easier to maintain if we had a quaternion class with its own set of quaternion-specific operators.

@RudolfWeeber
Copy link
Contributor

When I implemented the code rotating a single particle, I had the imperssion that Espresso uses quaternions the way, they are described in the Wikipedia.

I have reservations about changing the meaning of quaternion indices in a way that is visible to the user. That would be a really nasty silent interface change. Users specify quaternions directly, e.g., for virtual sites which have a fixed orientation with respect to the orientation of the central particle.

I see the benefit of replacing stuff by library code. On the other hand, I'm not sure the benefits in this particular case warrant the potential hazard a definition change can cause to users.
Particularly, since such a change doesn't provide significant new functionality to the user and the current code is well tested.

@bogdan-tanygin
Copy link
Contributor Author

In my opinion, the current formalism is espresso is not satisfactory. The indices do not match those from other libraries like boost::math::quaternion, and operations like rotation inversion and first/second derivative would probably be easier to maintain if we had a quaternion class with its own set of quaternion-specific operators.

@jngrad could you please specify which indices do not match? Is the boost quaternion "scalar i j k"?

@bogdan-tanygin
Copy link
Contributor Author

bogdan-tanygin commented Sep 14, 2019

please correct me if I'm wrong. I think, I've just found answer:
https://cs.brown.edu/~jwicks/boost/libs/math/quaternion/quaternion.html
... real and R_component_1 return the same value. ...
Hence, yes, should be "scalar i j k".

@jngrad
Copy link
Member

jngrad commented Sep 14, 2019

Nevermind, quaternion<T> & boost::math::quaternion::operator *=(quaternion<X> const & rhs) has the exact same formula as in our multiplication function if I re-arrange the terms properly. Both boost and espresso use the {scalar, vector} aka (w, i, j, k) format.

@bogdan-tanygin
Copy link
Contributor Author

great! So, after the work on #3164 I suggest to add some reference which align referred works Martys1999/Omelyan1998/sonnenschein1985 towards our implemented formalism. It can be e.g. Allen, Michael P., and Dominic J. Tildesley. Computer simulation of liquids. Oxford university press, 2017. considering highly cited textbooks. I can help with it, probably, this is just comments in the source code + documentation though..

bors bot added a commit that referenced this issue 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 <jgrad@icp.uni-stuttgart.de>
Co-authored-by: Kai Szuttor <kai@icp.uni-stuttgart.de>
@kodiakhq kodiakhq bot closed this as completed in #3392 Jan 3, 2020
kodiakhq bot added a commit that referenced this issue Jan 3, 2020
Fixes #2964

Description of changes:
 - code minor comment
 - docs minor change
 - bib ref to both

PR Checklist
------------
 - [ ] Tests?
   - [ ] Interface
   - [ ] Core 
 - [ ] Docs?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants