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

Define usage of NXtransformations and NXcoordinate_system(_set) #144

Conversation

lukaspie
Copy link
Collaborator

@lukaspie lukaspie commented Jan 9, 2024

Fixes #143.

  • in TRANSFORMATIONS depends_on, one can place either ".", or the path to an NX_coordinate_system
  • in NXcoordinate_system, there is a depends_on, and TRANSFORMATION field
  • 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
  • NX_coordinate_system_set contains NXcoordinate_systems
  • add example to doc of NXcoordinate_system_set with multiple NX_coordinate_system(s) and their transformations -> shall be handled in Implement base change directly in NXtransformations #235

EDIT: an example for converting from one CS to another by using NXcoordinate_system/(NXtransformations) can be found here.

Copy link
Collaborator

@rettigl rettigl left a comment

Choose a reason for hiding this comment

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

LGTM. The aspect of what it practically means if @depends_on points to an NXcoordinate_system off course depends on the actual implementation.
The aspect of converting the handedness of CO systems is not covered by NXtransformations, but I think we can just ignore this for now...

@domna domna added the mpes label Jan 10, 2024
base_classes/NXtransformations.nxdl.xml Outdated Show resolved Hide resolved
base_classes/NXtransformations.nxdl.xml Outdated Show resolved Hide resolved
contributed_definitions/NXcoordinate_system.nxdl.xml Outdated Show resolved Hide resolved
@lukaspie lukaspie mentioned this pull request Jan 15, 2024
Base automatically changed from mpes-refactor to fairmat January 15, 2024 16:20
@lukaspie
Copy link
Collaborator Author

lukaspie commented Feb 14, 2024

@sanbrock, @rettigl, and I had another discussion today and came up with the following idea.

  • redefine NXtransformations/AXISNAME/@depends_on such that it can point to an NXcoordinate_system as well. In that case, the base vectors x, y, z define the coordinate system change (aka base vector rotation) from another coordinate system to this CS.
  • add NXcoordinate_system/@depends_on: This defines which coordinate system this base transformation refers to. By default, it is (.) and points to the Mcstas coordinate system.
  • DO NOT use NXtransformations inside NXcoordinate_system because the three basis vectors describe this transformation anyway.
  • add offset to NXcoordinate_system: this allows for translational shifts of the coordinate system (so not just rotation of the base vector).
  • add an example of how the basis vectors and offset in NXcoordinate_system works.
  • Optional: drop NXcoordinate_system since it does not actually add any value other than being a container for all CSs.

For the future, @sanbrock suggested another option: add everything that is now in NXcoordinate_system to NXtransformations, thus making it possible to describe non-affine transformations and changes in handedness by NXtransformations. In that case, we would add the following to NXtransformations:

NXtransformations:
  BASE_COORDINATE_CHANGE(NX_NUMBER):
    @x(NX_NUMBER):
    @y(NX_NUMBER):
    @z(NX_NUMBER):
    @offset(NX_NUMBER):

What do you think @FAIRmat-NFDI/areab?

@lukaspie lukaspie linked an issue Feb 14, 2024 that may be closed by this pull request
@tomio13
Copy link
Collaborator

tomio13 commented Feb 14, 2024

Do I understand it correctly, that with this you stuffed an NXtransformation into the NXtransformation, the former one forcing a trasnlation shift along a vector, the second being whatever usual transformation (translation or rotation) it would have anyway been?

@lukaspie
Copy link
Collaborator Author

Do I understand it correctly, that with this you stuffed an NXtransformation into the NXtransformation, the former one forcing a trasnlation shift along a vector, the second being whatever usual transformation (translation or rotation) it would have anyway been?

No, there is no nesting of NXtransformations. The idea (please correct me if I am wrong @mkuehbach) is that in NXcoordinate_system, you have three base vectors that define this coordinate system with respect to the CS that you refer to with the @depends_on attribute. This would be a passive transformation.

Then, any AXISNAME in NXtransformations can point (via @depends_on) to one of the NXcoordinate_system instances that you define somewhere else in the tree and then you know that this transformation lives within that CS.

The main idea why we want to use NXcoordinate_system (instead of describing the CS also with NXtransformations) is that with the existing NXtransformations base class, it is not possible (to my understanding) to describe changes in handedness and non-affine transformations during a CS change.

@tomio13
Copy link
Collaborator

tomio13 commented Feb 14, 2024

So, basically complement the NXtransformations with a coordinate system defined by 3 base (unit) vectors. With this, transferring the so-far free text definition to this base class. The base vectors can be right-handed or left handed, and the vector in the NXtransformation is then meant to be in this coordinate system.

@mkuehbach
Copy link
Collaborator

mkuehbach commented Feb 14, 2024

NIAC conceptualized that NXtransformation can be connected into a graph with one single-directed edge between two nodes (NXtransformation) instances (a typical triplet). There is no parent-child relationship implied via this conceptually, therefore yeah no nesting of NXtransformations.

Now NXcoordinate_system and NXtransformations: What I proposed is indeed NXcoordinate_system as an explicit way to say I have that many coordinate systems with the i) possibility to define their base vectors explicitly as childs of respective NXcoordinate_system instance see fields x,y,z, ii) lately the connection between NXtransformations and NXcoordinate_system has been discussed rightly so, I support to define base vectors as childs in an NXcoordinate_system instance but then there have to be three disjoint NXtransformations instances one for each base vector (if parallelepiped is to be spanned).

In my opinion another possibility to achieve the same thing: State explicitly what your coordinate system is.
depends_on as an attribute of NXtransformations is reserved to chain NXtransformations, not to connect into NXcoordinate_system, however, I am not happy with this because stating that . is the end of a transformation chain also means where is this chain defined in, if nothing is stated McStas. But what if I have a community which does not want to use McStas in this case I see not a problem with the thought of using a triplet of NXtransformations inside NXcoordinate_system but than this triplet cannot be the end point as that is a chain with say one NXtransformation out and three in or vice versa, leaving an ill-defined state. But why can one not set an attribute the specifies explicitly in which CS an NXtransformation is living, i.e. I do not really see why one must not use depends_on to build a directed graph between an instance of NXcoordinate_system and an instance of NXtransformations.

@lukaspie
Copy link
Collaborator Author

lukaspie commented Feb 14, 2024

i) coordinate systems with the i) possibility to define their base vectors explicitly as childs of respective NXcoordinate_system instance see fields x,y,z, ii) lately the connection between NXtransformations and NXcoordinate_system has been discussed rightly so, I support to define base vectors as childs in an NXcoordinate_system instance but then there have to be three disjoint NXtransformations instances one for each base vector (if parallelepiped is to be spanned).

This is what I don't understand about NXcoordinate_system. From what you said here, I can (arbitrarily) define three vector x, y, and z, but how do I say with respect to what I define these? Would they not have to explicitly defined w.r.t. to McStas or another CS? My idea was that you define these vectors and use the depends_on attribute of NXcoordinate_system to say what these vectors are in another CS.

But why can one not set an attribute the specifies explicitly in which CS an NXtransformation is living, i.e. I do not really see why one must not use depends_on to build a directed graph between an instance of NXcoordinate_system and an instance of NXtransformations.

We also discussed this. The alternative would be to define a new attribute in NXtransformations (how about @coordinate_system) to explicitly define in which CS this particular NXtransformation is living.

@rettigl
Copy link
Collaborator

rettigl commented Feb 14, 2024

For orthonormal coordinate systems, one can can discribe it simply with one or more NXtransformation matrices, and the new base vectors are the MCstas base vectors times the resulting matrix, no? This is the use case I need. My idea is to create the NXcoordinate instance, and define the depends_on and NXtransformation chain within to define this matrix (even not give the vectors, as these result from the transformations).

depends_on as an attribute of NXtransformations is reserved to chain NXtransformations, not to connect into NXcoordinate_system

"." itself is not a transformation, but rather a coordinate system, so I would say we should extend the use of @depends_on to allow to point to other NXcoordinate system instances.

@sanbrock
Copy link

Discussing it with NIAC yesterday, it is indeed supported to extend NXtransformatinos so it does not only support rotation and translation but also "general" transformation described by an arbitrary transformation matrix (in fact x,y,z of our coordinate_system does describe such matrix by providing its column vectors). Hence, a base change is just yet another transformation type which can be part of the @depends_on chain wherever it is needed (not only at the end; and not only at a single point in the chain).
Note that CIF (which the idea of NXtransformation was built on) does have that capability and also supports working with fractional coordinates. It was also suggested to extend the dimensionality of NXtransformations to be able to support more dimensions than just 3. This could be implemented by introducing a symbol (e.g. 'n') representing the rank, so vectors shall follow that. In a coordinate transformation, we either give an n x n matrix and a translation vector, or directly an n+1 x n+1 augmented affine transformation matrix which can do translation, rotation, but also reflection, scale, and shear.

@mkuehbach
Copy link
Collaborator

mkuehbach commented Feb 15, 2024

Much of the last three comments I second. One point I would like to make more clear: It is alright that McStas is a convention some people wish to use. Fact is that it is one of many possible ones and NXcoordinate_system is an attempt to show that NeXus user are not necessarily required to use McStas if they dont want to.

On @lukaspie your point where is an instance of coordinate system embedded in: that is to some extend always arbitrary at the origin of the chain. A map of the globe is in world coordinate system where is that embedded in a cosmology-based reference frame but where is that embedded in... exactly it is defined as is. Therefore, we have the free-text fields in like "edge of the table etc".

Now on generalizing NXtransformation: Yeah that is probably a useful strategy for the future. The definition of say base vectors (vectors in general in maths) is within a reference frame, say we define a vector as a tuple of numbers in $\mathcal{R}^d$, again where is $\mathcal{R}^d$ defined in, the discussion ends as aforementioned. You define a few rules and then use only this ruleset to derive everything else. Conceptually we may ask is the concept of a transformation and a reference frame exactly the same. I argue no and that is exactly reason i) why I proposed NXcoordinate_system*. Reason ii) was that for many practical cases the discussions here are already way to detailed and not only from a political point we should first of all convince people to document their reference frame conventions at all and doing so explicitly. Ones this has been achieved with either transformations or coordinate systems, the discussion here is relevant. Then I think we have to make a difference between the concept of a reference frame and the concept of transformation happening within these reference frames or affecting the orientation of these reference frames.

@rettigl
Copy link
Collaborator

rettigl commented Jul 1, 2024

@lukaspie What is the status of this? As both NXxps and NXmpes_arpes use the transformations in NXcoordinate_system, I would suggest to merge this now (however, rn the transformations are actually not included!)

@lukaspie
Copy link
Collaborator Author

lukaspie commented Jul 1, 2024

@lukaspie What is the status of this? As both NXxps and NXmpes_arpes use the transformations in NXcoordinate_system, I would suggest to merge this now (however, rn the transformations are actually not included!)

My plan was to present/discuss this tomorrow in the TF meeting. @sanbrock actually had another idea how we could change NXtransformations, so we can just discuss it all together and then merge this PR afterwards.

EDIT: I added (NXtransformations) and depends_on to NXcoordinate_system again as discussed in the description of the PR.

@lukaspie lukaspie force-pushed the 143-proper-use-of-nxtransformations-and-nxcoordinate_system_set branch from cccc840 to 089841f Compare July 1, 2024 13:21
@lukaspie lukaspie force-pushed the 143-proper-use-of-nxtransformations-and-nxcoordinate_system_set branch from 089841f to 1e4929e Compare July 1, 2024 14:13
@lukaspie
Copy link
Collaborator Author

lukaspie commented Jul 2, 2024

Feedback from today's TF meeting:

  • handedness and x, y, z vectors are potentially reductant:

    • you can just do cross product of x and y and dot product with z -> if positive: right handed, if negative: left handed
    • question: what happens if handedness and the vector definitions are contradictory?
  • There is support for using NXcoordinate_system for describing actual orientations in space

    • especially useful for community-agreed standards (like VAMAS CS)
  • There is support for putting an instance of NXcoordinate_system as @depends_on in NXtransformations

    • if @depends_on=".", we are going back to mcstas
    • if you put @depends_on=any_coordinate_system(NXcoordinate_system), you essentially say that this is a point where you want to define x, y, z for a known (e.g, a community standard) CS
  • The coordinate systems need to be connected to the actual physical components in a meaningful way

    • this is something that is potentially missing

@rettigl based on these discussion in the TF, I would say we can merge this PR now and then figure out all the new discussions in #235. Do you agree?

@lukaspie lukaspie dismissed mkuehbach’s stale review July 3, 2024 12:58

either outdated or will be handled elsewhere

@lukaspie lukaspie marked this pull request as ready for review July 3, 2024 12:58
@lukaspie lukaspie merged commit 7380c8d into fairmat Jul 3, 2024
6 checks passed
@lukaspie lukaspie deleted the 143-proper-use-of-nxtransformations-and-nxcoordinate_system_set branch July 3, 2024 14:31
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.

Proper use of NXtransformations and NXcoordinate_system(_set)
6 participants