Add NXgoniometer base class

[rayosborn]

This PR adds a new base class that allows the goniometer angles used in a (usually single crystal) measurement to be defined according to a variety of naming conventions defined by an NXcite group. This group is used unofficially in the NXRefine package for reducing single crystal diffuse scattering (https://nexpy.github.io/nxrefine). In recent years, the NeXus convention has been to define a set of NXtransformations that obviate the need to define the meaning of such angles. This has the advantate that there is no universally accepted standard for naming them so that their meaning can be ambiguous. However, there are two disadvantages to this approach:

1. It is less intuitive for the user to check these angles against their own records of the experiment.
2. It assumes that the transformations defined in the file are the correct ones.

As an example of the second problem, in NXRefine, we discovered that out initial assumptions about the chain of transformation matrices was incorrect because the detector was decoupled from any two-theta arm. In stable experimental configurations, the transformation approach should work, but when experimenting with different goniometer and detector configurations, as well as different data reduction procedures, there are advantages in simply storing the angles as recorded by the beamline motors. Ideally, the meaning of these parameters is clearly described by the cited reference.

As a new base class, this requires formal approval by the NIAC.

[woutdenolf]

So we have several ROTATION_ANGLE and SAMPLE_TRANSLATION fields (can have any name). When I read such a file, how do I know the meaning of these fields? I'm guessing the fields convention and/or goniometer would provide this meaning based on the names? At least for humans, I'm not sure how this would work for software. Unless we compile a list of conventions.

[PeterC-DLS]

I can see how this group is useful to gather together the NXpositioner groups (or solely, links to their value fields) from each goniometer arm. However these fields are also linked to fields in NXtransformations groups for the sample and any detectors - consequently, they have corresponding attributes (@vector, @transformation_type, etc). It is an external matter to NeXus if these attributes are wrong: any analysis program should be able to override, ignore, or correct them.

[rayosborn]

So we have several ROTATION_ANGLE and SAMPLE_TRANSLATION fields (can have any name). When I read such a file, how do I know the meaning of these fields?

@woutdenolf, defining what the goniometer names mean was the purpose of the NXcite group. For example, in the case of the NXRefine package, it would refer to this page, where the rotation angle names chi, omega, phi, and theta are explained.

[rayosborn]

I can see how this group is useful to gather together the NXpositioner groups (or solely, links to their value fields) from each goniometer arm. However these fields are also linked to fields in NXtransformations groups for the sample and any detectors - consequently, they have corresponding attributes (@vector, @transformation_type, etc). It is an external matter to NeXus if these attributes are wrong: any analysis program should be able to override, ignore, or correct them.

@PeterC-DLS, my comment above applies here as well. For the reasons I gave at the top, I prefer that the analysis code decides how to handle transformations, although I am aware of reasons why the NXtransformations chain was originally adopted.

[phyy-nx]

When I was first reviewing NXmx I wondered why there was no formal NXgoniometer class and why we instead use a series of NXtransformations under the NXsample group. That always seemed unintuitive. Having a specific NXgoniometer class makes sense to me, and if approved, we might want to incorporate it into NXmx as an alternative way of representing a goniometer. I would want to review the axes specifications with some of my crystallographer colleagues though, since in XFEL-land where I have typically lived, we don't have goniometers. Tagging @graeme-winter for example.

[graeme-winter]

When I was first reviewing NXmx I wondered why there was no formal NXgoniometer class and why we instead use a series of NXtransformations under the NXsample group. That always seemed unintuitive. Having a specific NXgoniometer class makes sense to me, and if approved, we might want to incorporate it into NXmx as an alternative way of representing a goniometer. I would want to review the axes specifications with some of my crystallographer colleagues though, since in XFEL-land where I have typically lived, we don't have goniometers. Tagging @graeme-winter for example.

I don't believe we need an NXgoniometer class, in part for two reasons -

• the stack of axes reorienting the sample w.r.t. the lab coordinate system is already well defined through depends_on starting at the sample and working down
• more complex goniometers (e.g. the classic 6-axis) have the detector also on the goniometer

What would this add beyond opening the floodgates on arguments about the names of axes? 😉

If NXgoniometer were to exist I would not start using it in preference to the more abstract set-up we have right now

[graeme-winter]

Also:

we might want to incorporate it into NXmx as an alternative way of representing a goniometer

NO NO NO no no please NO!

We already have enough corner cases to support in analysis software without actively trying to introduce more

[phyy-nx]

Heh, roger that :)

[rayosborn]

@graeme-winter, the existence of a base class does not mean people are forced to use it, just as I am not forced to use the NXmx application definition, even though there is some overlap between single crystal diffuse scattering and macromolecular crystallography. Also, there shouldn't be any argument about the names of rotation angles, because the proposed base class allows them to be defined by different conventions, preferably with references to online documentation. I realize that this means our files are incompatible with a Dials-based analysis, just as they are incompatible with many other applications, but once (or perhaps assuming) this is approved, it will form the basis of an application definition describing how the NXRefine package analyzes the data.

[woutdenolf]

I suggested at some point to extend this from NXtransformations but AXISNAME@vector is required which would mean ROTATION_ANGLE and SAMPLE_TRANSLATION would need this attribute. And the entire point of the class is that we don't want to define this. The meaning of the rotations and translations is entirely defined by the convention (NXcite).

[woutdenolf]

Perhaps this class can be integrated with NXtransformations like we did with NXcoordinate_system.

[phyy-nx]

From Telco: consider adding NXtransformations as an optional group, and a doc string something like this:

"To specify the movement of the sample, either provide a full NXtransformations chain using NeXus laboratory conventions, or provide ROTATION_ANGLE and SAMPLE_TRANSLATION with a convention. In the latter case it's up to the software that reads the data to apply the sample movements according to the convention."

[rayosborn]

I have added the following paragraph to the NXDL file:
"If the goniometer settings are part of a transformation chain, it is possible to record them as NXtransformation groups instead of as fields. It is still recommended that the names of the groups adhere to the convention referenced in the NXcite group."

Fortunately, this inherits from the NXcomponent base class, which allows any NXtransformations groups, so the group will validate whether the angles are stored as fields or NXtransformations groups.

[phyy-nx]

As discussed on the Telco, this PR is ready for a vote. Please vote using emojis, 👍 for yes, 👎 for no, and anything else e.g. 👀 for abstain. Voting will close in 2 weeks.

Thanks!

[benajamin]

I think that this is a terrible proposal that goes against the core mission of the NeXus project. This proposal is a template for pointing at random articles for the meaning of the recorded rotations. That meaning should be incorporated into the NXgoniometer base class documentation. It is fine to have the base class define multiple schemes, and to add further schemes over time. But to simply provide a pointer to the literature is to accept infinite chaos as the norm. @graeme-winter is right to not want this - it is a lazy solution that will do more harm than good.

Please don't push out this rushed proposal - take the time to do it properly.

[rayosborn]

@benajamin, this has hardly been rushed. It has been discussed at several telcos over the last six months, so it would have been useful to get this feedback before we had a pending deadline that could sink it for another year. Allowing multiple schemes for naming goniometer angles is no different than in the NXmx class, which just puts a set of NXtransformation groups into an NXsample without specifying any naming convention at all. At least, this provides a mechanism for identifying what the goniometer angles correspond to by referencing the published literature. If there is a mistake in the NXmx transformations, there is no way for the user to identify it, since they have to reconstruct the meaning of the angle by inspecting the whole transformation chain. I realize that the DIALS project has sufficient expertise to make mistakes unlikely on instruments that they explicitly support, but not everything is going to be analyzed by DIALS and the transformations could be written by people with less expertise.

As a long-time experimenter, it is important to me that there is a mechanism for recording the instrumental parameters in my NeXus files, in a way that corresponds to how they are defined on a particular instrument. On Sector 6, those angles are chi, omega, phi, and theta, which are all defined in https://nexpy.github.io/nxrefine/introduction.html#experimental-geometry. I do not appreciate being told that we cannot use those angles until the NeXus committee has got around to adding our particular scheme some time in the next few years. This is not a recipe for chaos; it is a practical solution to the problem that the NeXus committee does not have the resources to define every possible goniometer scheme used in the wild.

The alternative is the NXmx solution. Personally, I am uncomfortable with mixing metadata storage with data analysis in this way. For example, we discovered after the fact that a particular set of transformations that we had been using were incorrect because they assumed the detector was on a two-theta arm. We were able to fix that in our code without invalidating the file. However, I respect the right of the MX community to integrate their NeXus files into their data analysis procedure in the way that makes most sense to them. The NXgoniometer class offers a viable alternative that users will immediately understand, reinforced by online documentation that we cannot hope to replicate in a rigorous way in the NeXus standard.

[PeterC-DLS]

I think a positioner group base class is more general and can encapsulate other equipment that position samples and/or detectors (robot arms, hexpods, etc).

By not recommending that physical meanings of the labelled positions recorded in some mathematical way (adding NXtransformations attributes to the fields) means that this class is not specified tightly enough to be machine readable as there's no standard to parse NXcite - it's not generally interoperable.

[rayosborn]

@PeterC-DLS, once again, let me point out that this PR was submitted on May 1 and has been discussed at numerous Telcos since. Raising objections about the underlying premise of the base class is not, I believe, very helpful at this stage.

I think this complaint is conflating the role of base classes and application definitions. Files that conform to application definitions have to be machine-readable by their corresponding applications. For example, NXmx files are readable by DIALS and other applications that support the NXmx standard. These files don't have to be readable by other applications, such as NXRefine, which uses (at the moment, unofficially) a NXgoniometer group. I am preparing a NXRefine application definition, which will specify the names of the angles it requires. Files written according to this application definition will be machine-readable by the NXRefine application and any other applications that choose to support it. I do not expect them to be readable by DIALS.

Base classes do not have to be machine-readable in the sense that I believe @PeterC-DLS means. His complaint seems to imply that it is a requirement of the NeXus standard that an entire data reduction workflow can be reconstructed by reading any file, even in the absence of an application definition. As a corollary, it would require that the only NeXus-conformant files are those that use NXtransformations, which would render every single file I have ever written invalid, along with probably 99% of all other NeXus files.

[phyy-nx]

The PR did not get enough votes but a consensus here is highly desirable. We will discuss the best course of action on the next Telco.

[rayosborn]

@phyy-nx, I think we need to have a rule that once something has been put to a vote, there is no further discussion while the vote proceeds. Once a PR has been submitted, there should be a reasonable period for people to make comments online alongside discussions at two or more Telcos. Once you, as chairman, feel that there is a consensus, then you call a vote and all online discussions should stop until the vote is over.

I believe we had a consensus at the last Telco, but this vote was sunk by two negative comments written after the Telco was over. I attempted to answer both of their criticisms but have yet to receive a reply to either one. That is why I believe that the time for debate has to be before you call a vote, because comments during the voting period just cause confusion and doubt, rendering all our previous discussions pointless.

[phyy-nx]

Your concerns are similar to mine, we should review our policies. I plan on meeting with @sanbrock to review them and come up with a proposal.