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

Updates NXtransformations docs #114

Merged
merged 3 commits into from
Jan 9, 2024
Merged

Updates NXtransformations docs #114

merged 3 commits into from
Jan 9, 2024

Conversation

domna
Copy link

@domna domna commented Nov 8, 2023

Based on this PR: nexusformat#1278 and this issue nexusformat#1269.

Fixes #88

@domna domna requested a review from mkuehbach November 8, 2023 17:17
Copy link
Collaborator

@mkuehbach mkuehbach left a comment

Choose a reason for hiding this comment

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

Most changes just to check again carefully, for transformations it is always better to have as many eyes reviewing it as possible

@transformation_type=translation
@vector=[0 0 1]
@units=mm
omega=174
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we really sure that the order is not flipped between i) the initial definition of T1 depends on T2 depends on T3 ii) the the statement in the code block that omega depends on "." but iii) R_omega is shown first on the left-hand side of the equation introducing example one:
R(omega) ... R(chi) . R(varphi) However, this seems right when considering that depends_on . is the terminator of the chain i.e. the statement take the result as you have obtained until now when you executed the transformations and it will be your results Xlab, i.e. the coordinate from within sample actively rotated and relocated to Xlab


**Example 2: different coordinate system**

Define a laboratory reference frame with the X-axis along the beam and the Z-axis opposite to the direction of gravity.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here should be a note that NXcoordinate_system is another possibility to define such a "laboratory reference frame"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not like at all that we never explicitly tell whether the here always Cartesian ref frames are considered always right or left handed ones surplus which direction of rotation we consider as positive clock/anti-clockwise

Copy link
Collaborator

Choose a reason for hiding this comment

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

X-axis along the beam is not specific enough but +X points in parallel and into the same direction as the propagating beam because otherwise the sign remains unclear or just implicitly defined, same so the z-axis might be parallel to gravity but which sign along z is positive in the direction of gravity or vice versa?


* *transmission*:
* point detector in the beam
* 20 cm downstream from the sample (the origin of the reference frame)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the origin of WHICH reference frame (this means this lab frame we define, right?)


The coordinates of the point detectors in the laboratory reference frame are

* *transmission*: :math:`X_\text{lab} = T_x(20) . X_d`
Copy link
Collaborator

@mkuehbach mkuehbach Nov 8, 2023

Choose a reason for hiding this comment

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

Connection between X_d and the above-mentioned thought experiment of the lab ref frame unclear I think X_d has to be X_s(ample)? here, no

specification.
**Transformation chain**

The entry point of a chain of transformations is a field called ``depends_on``
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somewhere in this document we say depends_on . is the end of the transformation chain but this wording here implies it is the entry point i.e. the start of the transformation chain????

* *vertical*:
* point detector 10 cm downstream from the sample
* making an angle of 5 degrees with the beam w.r.t. the sample
* positioned in the XZ-plane above the XY-plane
Copy link
Collaborator

Choose a reason for hiding this comment

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

horribly complicated but maybe correct XZ plane living in which reference frame?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Proliferate changes in the NXDL

@depends_on=/entry/coordinate_system/beam
@units=cm
@vector=[1 0 0]
coordinate_system:NXtransformations
Copy link
Collaborator

Choose a reason for hiding this comment

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

why would beam depends_on gravity why do I think NXcoordinate_system is cleaner/more explicit?

@domna domna requested a review from lukaspie January 9, 2024 15:48
Copy link
Collaborator

@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

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

Further changes should be made together with #144

@domna domna dismissed mkuehbach’s stale review January 9, 2024 16:09

Will be handled later in the NIAC PR

@domna domna force-pushed the new-transformation-docs branch from e0d2283 to ea24123 Compare January 9, 2024 16:09
@domna domna merged commit 64ddaf6 into fairmat Jan 9, 2024
6 checks passed
@domna domna deleted the new-transformation-docs branch January 9, 2024 16:15
lukaspie pushed a commit that referenced this pull request Sep 23, 2024
* Updates NXtransformations docs

* Manually set to lower case true

* Do a forward-backward nyaml cycle for NXtransformations
# Conflicts:
#	base_classes/nyaml/NXtransformations.yaml
lukaspie pushed a commit that referenced this pull request Sep 24, 2024
* Updates NXtransformations docs

* Manually set to lower case true

* Do a forward-backward nyaml cycle for NXtransformations
# Conflicts:
#	base_classes/NXtransformations.nxdl.xml
#	base_classes/nyaml/NXtransformations.yaml
lukaspie pushed a commit that referenced this pull request Oct 16, 2024
* Updates NXtransformations docs

* Manually set to lower case true

* Do a forward-backward nyaml cycle for NXtransformations
# Conflicts:
#	base_classes/nyaml/NXtransformations.yaml
lukaspie pushed a commit that referenced this pull request Oct 17, 2024
* Updates NXtransformations docs

* Manually set to lower case true

* Do a forward-backward nyaml cycle for NXtransformations
# Conflicts:
#	base_classes/nyaml/NXtransformations.yaml
lukaspie pushed a commit that referenced this pull request Jan 16, 2025
* Updates NXtransformations docs

* Manually set to lower case true

* Do a forward-backward nyaml cycle for NXtransformations
# Conflicts:
#	base_classes/nyaml/NXtransformations.yaml
lukaspie pushed a commit that referenced this pull request Feb 7, 2025
* Updates NXtransformations docs

* Manually set to lower case true

* Do a forward-backward nyaml cycle for NXtransformations
# Conflicts:
#	base_classes/nyaml/NXtransformations.yaml
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 this pull request may close these issues.

Get latest NXtransformations docs
3 participants