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

Implement base change directly in NXtransformations #235

Draft
wants to merge 2 commits into
base: fairmat
Choose a base branch
from

Conversation

lukaspie
Copy link
Collaborator

@lukaspie lukaspie commented Jun 3, 2024

This is an add-on to #144 which essentially incorporates NXcoordinate_system directly within NXtransformations.

There are three changes to NXtransformations:

  1. transformation_type may be base_change, in which case a matrix containing n_dim column vectors of n_dim length should be supplied.
  2. an attribute matrix is added to AXISNAME which shall contain n_dim column vectors of n_dim length. This is to be used to provide a matrix going from one coordinate system going to the other.
  3. an attribute direction is added to AXISNAME that provides a human-readable field which direction the axis points. This is to be used to a define a reference frame (i.e, for axes with no transformation_type). If matrix is filled, then this field shall contain the direction of all n-dim matrix elements.

Examples:

  1. There is an example in the NXtransformation docs.
  2. You can download an example for the CS change in NXtransformations here.

@lukaspie lukaspie self-assigned this Jun 3, 2024
@lukaspie lukaspie added enhancement New feature or request needs-discussion labels Jun 3, 2024
@lukaspie lukaspie force-pushed the base-change-within-nx-transformations branch from 3c61c82 to bd8a476 Compare July 1, 2024 14:13
@lukaspie
Copy link
Collaborator Author

lukaspie commented Jul 2, 2024

Feedback from today's PR:

  • translation/rotation does not cover change in handedness
    -> this is why we need to have a 4-dimensional matrix
  • This matrix should not be ndim x ndim, but rather (ndim + 1) x (ndim + 1)
  • @matrix should reflect that the actual 4D transformations are different in left/right handed CS
    • we should add the actual transformations that are applied for both systems into the NXtransformations docstring
  • we should somehow use the same notation for coordinate system and @transformation_type=base_change

@lukaspie
Copy link
Collaborator Author

lukaspie commented Jul 3, 2024

For reference, also copying here from #249:

The sense of "positive" rotation changes with the handedness of the CO system, i.e. one needs other rotation matrices depending on the handedness of the system.

If you think about a rotation around the z-axis from the perspective of viewing from the origin towards the positive end of the z-axis, you have:

  • Right-Handed Coordinate System: positive rotation is counterclockwise.
$$ R_z^{\text{right}}(\alpha) = \begin{pmatrix} \cos\alpha & -\sin\alpha & 0 \\\ \sin\alpha & \cos\alpha & 0 \\\ 0 & 0 & 1 \end{pmatrix} $$
  • Left-Handed Coordinate System: Positive rotation is clockwise.
$$R_z^{\text{left}}(\alpha) = \begin{pmatrix} \cos\alpha & \sin\alpha & 0 \\\ -\sin\alpha & \cos\alpha & 0 \\\ 0 & 0 & 1 \end{pmatrix}$$

For any arbitrary rotation around axis $\mathbf{u} = (u_x, u_y, u_z)$ by an angle $\theta$, the rotation matrix is:

  • For Right-Handed Coordinate System:
$$R_{\text{right}} = \begin{pmatrix} \cos\theta + u_x^2 (1 - \cos\theta) & u_x u_y (1 - \cos\theta) - u_z \sin\theta & u_x u_z (1 - \cos\theta) + u_y \sin\theta \\\ u_y u_x (1 - \cos\theta) + u_z \sin\theta & \cos\theta + u_y^2 (1 - \cos\theta) & u_y u_z (1 - \cos\theta) - u_x \sin\theta \\\ u_z u_x (1 - \cos\theta) - u_y \sin\theta & u_z u_y (1 - \cos\theta) + u_x \sin\theta & \cos\theta + u_z^2 (1 - \cos\theta) \end{pmatrix}$$
  • For Left-Handed Coordinate System:
$$R_{\text{left}} = \begin{pmatrix} \cos\theta + u_x^2 (1 - \cos\theta) & u_x u_z (1 - \cos\theta) + u_y \sin\theta & u_x u_y (1 - \cos\theta) - u_z \sin\theta \\\ u_z u_x (1 - \cos\theta) - u_y \sin\theta & \cos\theta + u_z^2 (1 - \cos\theta) & u_z u_y (1 - \cos\theta) + u_x \sin\theta \\\ u_y u_x (1 - \cos\theta) + u_z \sin\theta & u_y u_z (1 - \cos\theta) - u_x \sin\theta & \cos\theta + u_y^2 (1 - \cos\theta) \end{pmatrix}$$

This is also what @domna implemented in the nexus3d package here.

NXtransformations/@vector with @transformation_type=rotation explicitly mentions "right-handed rotation with increasing angle". In the future, it would be best if we would allow both left- and right-handed rotations and mention in the docstring what that means in terms of the actual rotations. There, we should probably also state the orientation that we choose. Namely, we are looking from the origin towards positive axis values. This is also in line with what is the current definition of the NeXus coordinate system.

See also this and this link.

@lukaspie lukaspie force-pushed the base-change-within-nx-transformations branch from bd8a476 to d53cf0f Compare August 30, 2024 14:48
@lukaspie lukaspie force-pushed the base-change-within-nx-transformations branch from 3defd6e to 627a203 Compare September 11, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant