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

Fairmat 2024: use NXcoordinate_system together with NXtransformations #1415

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

lukaspie
Copy link
Contributor

@lukaspie lukaspie commented Sep 23, 2024

This PR is concerned with coordinate systems and the transformations that are intricately linked to them. It was started for multiple reasons:

  • There is the frequent need to explicitly describe which coordinate system is used in an experiment.
  • NeXus by default uses a coordinate system that is the same as that used by McStas. However, this can be limiting:
    • there are now several techniques coming in where there is no explicit beam defined: temperature-dependant IV measurements, scanning probe microscopy (STM, AFM, Atom Probe, or multi-beam EM), etc.
    • in some communities, there already exists an agreement on which coordinate system shall be used, which is different than what NeXus uses. As an example, there is an existing ISO standard for data transfer in surface chemical analysis that uses a coordinate system that is left-handed and defined based on the sample stage. It would be good if in an application definition (e.g. NXxps) one could explicitly state which coordinate system is to be used. There has already been some discussion on this, see NXmpes #1464 (comment)
      • Specifically on this, for the XPS community, there are certain angles (e.g. analyser-to-beam) that are needed to perform rigorous analysis. It would be helpful if one could write these in the existing coordinate system (as in the ISO standard), without needing to transform back from the NeXus coordinate system.
  • NeXus (with its concept of NXtransformations) does already allow to define a specific coordinate system (see e.g. discussion in NXtransformations: clarify that these are active transformations + example #1278). But, this has several limitations:
    • these transformations always depend on a tool that can read them out and perform the transformations itself. We would also like to allow that one can give an coordinate system explicitly, both in a machine and human readable way
    • left-handed CSs are currently not supported
    • fractional coordinates are not supported

Therefore, we introduce in this PR two new base classes: NXcoordinate_system and NXrotation_conventions.

Explanation of using NXcoordinate_system and NXtransformation:

  • An application defintion contains one or more of NXcoordinate_system right below NXentry. See how to use different number of instances NXcoordinate_system from its lengthy docstring.
  • in NXcoordinate_system, there is a depends_on, and (NXtransformation) group
  • in NXtransformation AXISNAME depends_on, one can place either ".", or the path to an NX_coordinate_system
  • if a depends_on attribute or field links to a NXcoordinate_system, it should pick the respective depends_on field in that class, and apply the specified TRANSFORMATIONS

Example of how this is implemented in a (proposed) application definition: NXXps. Note that this one still uses the logical grouping NXcoordinate_system_set, but we removed this because it is not really needed. We would simply have xps_coordinate_system(NXcoordinate_system) directly under NXxps/NXentry.

@lukaspie lukaspie marked this pull request as draft September 23, 2024 16:07
@lukaspie lukaspie force-pushed the fairmat-2024-nxtransformations branch from d5b73b5 to 677b6a1 Compare September 23, 2024 16:19
@lukaspie lukaspie marked this pull request as ready for review September 23, 2024 16:19
@sanbrock sanbrock mentioned this pull request Sep 29, 2024
@lukaspie lukaspie linked an issue Sep 29, 2024 that may be closed by this pull request
@lukaspie lukaspie mentioned this pull request Oct 8, 2024
@lukaspie lukaspie force-pushed the fairmat-2024-nxtransformations branch 2 times, most recently from 52e7bac to 7f010d8 Compare October 17, 2024 14:15
@lukaspie lukaspie added NIAC vote needed PR needs an approving vote from NIAC before merge discussion needed labels Oct 17, 2024
@lukaspie
Copy link
Contributor Author

lukaspie commented Dec 6, 2024

Copying here discussion that was buried in #1464 for clarity:

LP: we shall align to and extend the existing ISO standard for XPS. The concept of coordinate systems allow connecting multiple conventions.
PM: Which use cases needs a different coordinate system?
LP: ISO standard requires a different coordinate system
SB: Another example is when NeXus coordinate system being linked to incident beam does not exists because no beam exists in a specific experiment type, like temperature dependent IV measurement.
PM: shall we make this a standard, so all sw shall implement the ability of changing coordinate systems?
HB: NeXus shall allow multiple coordinate systems, as MX is also using it and other fields also require that. It is preferred to have a clearly described way how to do that.
PC: NeXus uses McStas as default, but other coordinate systems can also be used. Important is that the use of NXtransformations shall be consistent throughout the datasets.
SB: This is exactly what the proposal is about: provide a simple solution either on how to use NXtransformations when a base change is required to connect two coordinate systems, or on describing a coordinate system which is used in the definition and data file.
PM: Cannot we use a label for this purpose?
SB: Indeed that is what hapenning: Currently, the NXtransformations chain is followed along @depends_on until it ends with '.'. The proposal is that instead of ending with a '.', it could end with a label pointing to a Coordinate System which either describes a coordinate system on its own, or connetcs it to another coordinate system using NXtransformations.

@lukaspie lukaspie changed the title Fairmat 2024: mention NXcoordinate_system in NXtransformations Fairmat 2024: use NXcoordinate_system together with NXtransformations Dec 6, 2024
@mkuehbach
Copy link
Contributor

"_set" is a restricted keyword thus NXcoordinate_system_set class needs a name change.

NXcoordinate_system_set is meant to provide a specialized NXobject with several commonly used NXcoordinate_system conventions with their own specialized NXcoordinate_system (CS) such that this collection can directly be used in appdefs instead of having each app demanding that if they wish to define one or multiple of the CS defined in NXcoordinate_system need to type the definitions again.

Possibilities:

  1. Rename NXcoordinate_system_collection (lousy but straightforward)
  2. Move top-level docstring of CS_set class to CS (+ would not loose context but - would loose predefined CS (processing_reference_frame, sample_reference_frame, etc.)
  3. Similar to 2. delegate the definition of several CS to each appdef with maxOccurs not constraints, in practice people will then likely write CS1, CS2, CS3 etc.

@mkuehbach
Copy link
Contributor

Some comments from @PeterC-DLS made in #1421 will be taken over here

@nexusformat nexusformat deleted a comment from mkuehbach Jan 16, 2025
@nexusformat nexusformat deleted a comment from mkuehbach Jan 16, 2025
@lukaspie lukaspie force-pushed the fairmat-2024-nxtransformations branch from e9ce133 to c72d8c8 Compare January 16, 2025 14:11
<doc>
Coordinate system type.
</doc>
<enumeration>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"An open enumeration?"

Comment originally posted by @PeterC-DLS in #1421 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can only be done after addressing #1521

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about

<enumeration open="True">
    <item value="cartesian"/>
    <item value="polar"/>
    <item value="cylindrical"/>
    <item value="spherical"/>
    <item value="homogeneous "/>
</enumeration>

</field>
<field name="x" type="NX_NUMBER" units="NX_LENGTH">
<doc>
Basis unit vector along the first axis which spans the coordinate system.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"You should explain that this coordinate system is defined relative to the laboratory frame."

Comment originally posted by @PeterC-DLS in #1421 (comment)

Copy link
Contributor

@mkuehbach mkuehbach Feb 14, 2025

Choose a reason for hiding this comment

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

Mhh, puzzled by this one: McStas defines a coordinate system, where does this CS life in?
Likely somewhere but for NeXus irrelevant because the assumption is,
McStas defines the laboratory frame: i.e. it is THE root coordinate system it defines i.e. the relevant Euclidean space laboratory frame.
So far, NXtransformations then act as "active" transformations which relocate, rotate objects in that fixed McStas frame.
Now people may wish to use NXcoordinate_system to define N instances of CSs (with these maybe different than McStas). Where do these CSs life in? Why should every of these N CSs necessarily be defined in the laboratory frame (McStas)? Why can at least not one of these N define the laboratory frame itself. Sure, the other CSs of the N may define relative to the laboratory frame, possibly then expressed via using NXtransformations to either actively rotate the base vectors of each CSs into McStas or vice versa.

In summary, I think I am puzzled and feel the comment that this Cs is defined relative to the lab frame not necessarily applies, at least it confuses me.

the i-axis in reciprocal space.
</doc>
<dimensions rank="1">
<dim index="1" value="3"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Why restrict yourself to 3 dimensions?"

Comment originally posted by @PeterC-DLS in #1421 (comment)

Copy link
Contributor

@mkuehbach mkuehbach Feb 14, 2025

Choose a reason for hiding this comment

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

Good point:

  1. If I would not specify a dimensionsType, this means that the value is not necessarily a scalar right?
  2. Alternatively, we could define a symbol "d" like we use for the NXcg_* base classes and then define
    dim index="1" value="d", also then one should add a docstring comment d>=1

I prefer solution 2, which do you prefer?

* Updates NXtransformations docs

* Manually set to lower case true

* Do a forward-backward nyaml cycle for NXtransformations
# Conflicts:
#	base_classes/nyaml/NXtransformations.yaml
@lukaspie
Copy link
Contributor Author

I wanted to add to the discussion on handedness of coordinate system and the implied rotations.

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}$$

Wouldn't that have to be the rotations that we represent by the vectors in NXtransformations?

Currently, the latter transformation is not possible with the definition of NXtransformations, which explicitly talks about right-handed transformations only. These matrices should probably be part of the docstring (either in NXcoordinate_system or NXtransformations) if we want to allow both left- and right-handed CSs and rotations. There (or in NXrotation_conventions or a similar class), 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.

@sanbrock
Copy link
Contributor

Telco: suggestions made shall be implemented, and after a positive feedback from Wout, we could run a vote.

<doc>
Which sign convention is followed when converting orientations
between different parameterizations/representations according
to convention 6 of DOI: 10.1088/0965-0393/23/8/083501.
Copy link
Contributor

@PeterC-DLS PeterC-DLS Feb 13, 2025

Choose a reason for hiding this comment

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

There does not seem to be an explicit sentence on what convention 6 is in the cited paper. Maybe equation 14 is what needs to be cited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, there is not convention 6. Changed to equation 14 as suggested.

possible as to which of these coordinate system sets and instances
apply or take preference.
While these ambiguities should be avoided at all costs, the
opportunity for multiple sets and their instances enables to
Copy link
Contributor

Choose a reason for hiding this comment

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

Reword with the removal of "sets"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

apply or take preference.
While these ambiguities should be avoided at all costs, the
opportunity for multiple sets and their instances enables to
have branch-specific coordinate system conventions which are
Copy link
Contributor

Choose a reason for hiding this comment

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

branch? Do you mean of a tree hierarchy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. I also made the language a bit less harsh here, as discussed below.

Changed to
"While these ambiguities should be avoided if possible, the opportunity for instances enables to have coordinate system conventions that are specific for some part of the NeXus tree. This is especially useful for deep groups where multiple scientific methods are combined or cases where having a definition of global conversion tables how to convert between representations in different coordinate systems is not desired or available for now."

that all branches (to arbitrary depth) use either the only
``NXcoordinate_system`` instance in the tree or the application
definition is considered underconstrained. In the latter case, which
should be avoided at all costs, McStas is assumed again.
Copy link
Contributor

Choose a reason for hiding this comment

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

This wording is a bit too strong as it condemns all existing files!

Copy link
Contributor Author

@lukaspie lukaspie Feb 14, 2025

Choose a reason for hiding this comment

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

You are right. Changed to "In the latter case, which ideally should be avoided, McStas is assumed again. Note that this is the case for all files that were created before NXcoordinate_system was added."

<field name="x" type="NX_NUMBER" units="NX_LENGTH">
<doc>
Basis unit vector along the first axis which spans the coordinate system.
This axis is frequently referred to as the x-axis in real space and
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the "real" space?

Copy link
Contributor

Choose a reason for hiding this comment

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

Google: "what is the opposite to reciprocal space"

The space described by q is called the Fourier space (the q-space) or the reciprocal space. The term “reciprocal” is used because q has the unit of the reciprocal length, i.e., l/λ. On the other hand, the space defined by r is called the real space.

Does writing Euclidean space clarify the situation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed enhancement help wanted NIAC vote needed PR needs an approving vote from NIAC before merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NXtransformations
5 participants