-
Notifications
You must be signed in to change notification settings - Fork 56
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
base: main
Are you sure you want to change the base?
Fairmat 2024: use NXcoordinate_system together with NXtransformations #1415
Conversation
d5b73b5
to
677b6a1
Compare
52e7bac
to
7f010d8
Compare
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. |
"_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:
|
Some comments from @PeterC-DLS made in #1421 will be taken over here |
e9ce133
to
c72d8c8
Compare
<doc> | ||
Coordinate system type. | ||
</doc> | ||
<enumeration> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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"/> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point:
- If I would not specify a dimensionsType, this means that the value is not necessarily a scalar right?
- 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
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:
For any arbitrary rotation around axis
Wouldn't that have to be the rotations that we represent by the vectors in Currently, the latter transformation is not possible with the definition of |
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
This PR is concerned with coordinate systems and the transformations that are intricately linked to them. It was started for multiple reasons:
NXxps
) one could explicitly state which coordinate system is to be used. There has already been some discussion on this, see NXmpes #1464 (comment)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:Therefore, we introduce in this PR two new base classes:
NXcoordinate_system
andNXrotation_conventions
.Explanation of using
NXcoordinate_system
andNXtransformation
:NXcoordinate_system
right belowNXentry
. See how to use different number of instancesNXcoordinate_system
from its lengthy docstring.NXcoordinate_system
, there is adepends_on
, and(NXtransformation)
groupNXtransformation
AXISNAME
depends_on
, one can place either"."
, or the path to anNX_coordinate_system
depends_on
attribute or field links to aNXcoordinate_system
, it should pick the respectivedepends_on
field in that class, and apply the specifiedTRANSFORMATIONS
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 havexps_coordinate_system(NXcoordinate_system)
directly underNXxps/NXentry
.