add depends_on and related to all component type classes #897

mkoennecke · 2021-03-23T10:04:09Z

Fixes #681, This is the first version of adding NXtransformations, depends_on and NXoff_geometry to all base classes needing them.

…ave a depends_on field and NXtransformations group. Done until NXinsertion_device in the base class list.

…all base classes

base_classes/NXaperture.nxdl.xml

base_classes/NXattenuator.nxdl.xml

PeterC-DLS · 2021-03-23T10:25:43Z

base_classes/NXattenuator.nxdl.xml

+  </doc>
+</field>
+
+<group name="transformations" type="NXtransformations" minOccurs="0">


Is this name fixed?

I thought it is a good idea to have that in the same place in each group. Is there a use case for another name?

The name is not important from a parsing perspective because you are always following a depends_on path to find it anyway. I also can't think of a use case for allowing a different name though.

@prjemian Can you chime in on this? From session G of CodeCamp2021, we are split on this issue. Whether to specify the name as transformations or leave it optional. (Note, nobody had a really strong opinion :)

IMO, not sure why it is necessary to require the exact name. Within the BES Bluesky communitym there is a group looking at XPCS data and has begun learning how to apply NX transformations. They have assumed the name is not required to be exactly transformations. Thus, in any NXDL spec, the name could be omitted unless there are two or more NXtransformations groups in the same directory. In such case, they need names to avoid ambiguity.

For the crystallography community, prescriptive naming is not likely to work.

On the matter of deprecating distance in favor of deriving it from the axis chain, that also is not likely to work, since distance, wavelength and beam center are essential data for
processing. The best we are likely to be able t do is to make it clear when they are derived versus when they are directly observed and make derived the default.

base_classes/NXaperture.nxdl.xml

matthew-d-jones · 2021-03-23T11:25:47Z

base_classes/NXbeam_stop.nxdl.xml

+    the instrument. 
+  </doc>
+</group>
+<group type="NXoff_geometry" minOccurs="0">


Or "NXcylindrical_geometry"

matthew-d-jones · 2021-03-23T11:30:36Z

base_classes/NXtransformations.nxdl.xml

+		* transformation_type
+		* depends_on
+
+		as needed.


Was it agreed by NIAC to prefer using these names? I sometimes use the name of a motion axis device for example.
Should we also explicitly mention that an NXlog can be used in place of a dataset?

? These are the standard transformation related attribute types.

At the beginning of the code camp we discussed this issue and decided to move all the older NeXus
positioning fields into NXtransformations. The use case you describe, an axis name called hugo with a very weird operation, is still possible.

The NXlog is a good point and I see where I can fit this in.

Sorry, by "these names" I meant "distance", "height", etc.

phyy-nx · 2021-03-23T17:10:53Z

base_classes/NXdetector.nxdl.xml

+  <doc>
+    NeXus positions components by applying a set of translations and rotations 
+    to apply to the component starting from 0, 0, 0. The order of these operations 
+    is critical and form a what NeXus calls a dependency chain. The depends_on 


Should read "and forms what NeXus..."

PeterC-DLS · 2021-03-23T22:08:35Z

base_classes/NXtransformations.nxdl.xml

+	<dimensions rank="1">
+	  <dim index="1" value="3" />
+	</dimensions>
+	<enumeration>


This attribute is defined as having a number type thus can probably cannot be an enumeration.

That's not quite right: arrays are not allowed as enumeration items.

PeterC-DLS · 2021-03-24T08:47:33Z

base_classes/NXtransformations.nxdl.xml

@@ -176,6 +192,207 @@
 			differences.  Use of ``AXISNAME_end`` is recommended.
 		</doc>
 	</field>
+    <field name="distance" type="NX_FLOAT" units="NX_LENGTH" minOccurs="0">


This fixes the coordinate frame's z axis direction. I think the frame should be left as a choice for beamlines/facilities.

yayahjb

See comments on prescriptive naming and keeping explicit distance, beam center and wavelength, and note that the enumeration issue still needs to be resolved, but all that being said, it really is time to make use of NXtranformations the norm.

…line components

phyy-nx · 2021-03-29T21:53:17Z

I believe that what @mkoennecke has done is to add depends_on and NXtransformations where they were missing, which I'm quite pleased about. However, as discussed in one of the code camp sessions, going further and deprecating distance and beam center fields is likely not necessary. I've reverted the deprectations and replaced them with comments here:

issue-681...issue-681-aaron

Further, while I was working on this, I see that @mkoennecke added the deprecated fields explicitly to NXtransformations as example names for beamline components. For example, compare this removal of distance to where it was added as an example axis name to NXtransformations.

I think the additions to NXtransformations aren't needed in formal NXdl language. I'd instead like to see examples as text, such as what @yayahjb and I are hoping to do when resolving #412. That would make it more clear that axis names and conventions aren't proscriptive. But as that's more work, I'd instead propose reverting most of the changes to NXtransformations as the next step in this PR. Would love to hear any feedback though.

Thanks!

yayahjb · 2021-03-30T01:17:35Z

Aaron is right -- Herbert

…

On Mon, Mar 29, 2021 at 5:53 PM Aaron S. Brewster ***@***.***> wrote: I believe that what @mkoennecke <https://github.com/mkoennecke> has done is to add depends_on and NXtransformations where they were missing, which I'm quite pleased about. However, as discussed in one of the code camp sessions, going further and deprecating distance and beam center fields is likely not necessary. I've reverted the deprectations and replaced them with comments here: issue-681...issue-681-aaron <issue-681...issue-681-aaron> Further, while I was working on this, I see that @mkoennecke <https://github.com/mkoennecke> added the deprecated fields explicitly to NXtransformations as example names for beamline components. For example, compare this removal of distance <https://github.com/nexusformat/definitions/pull/897/files#diff-958495767f26dd709fe7d1ab478f93f1183a252690722aeed15013b7b9f4ec81R46> to where it was added as an example axis name <https://github.com/nexusformat/definitions/pull/897/files#diff-d5885ae955bdaadaf3e76bec7bfc38ffa4225cb3072708adaeee02116912726bR197> to NXtransformations. I think the additions to NXtransformations aren't needed in formal NXdl language. I'd instead like to see examples as text, such as what @yayahjb <https://github.com/yayahjb> and I are hoping to do when resolving #412 <#412>. That would make it more clear that axis names and conventions aren't proscriptive. But as that's more work, I'd instead propose reverting most of the changes to NXtransformations as the next step in this PR. Would love to hear any feedback though. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#897 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB6EAPOIQASYHBAIL2XCALTGDZFXANCNFSM4ZU3CGUQ> .

mkoennecke · 2021-03-30T06:36:16Z

Hmmh, IMHO we should not give up to quickly on names for transformations. I am in favor to keep them at least as suggestions. Some of them are very old names, with NeXus since its inception. These are things like distance, rotation_angle, polar_angle, height and such. Then there are names which never got popular like azimuthal_angle, meridional_angle. May be drop those. We not only have to cater for the anarchic MX community but also for people who are relatively new to NeXus and would prefer a little hand holding when choosing a name for say: a translation in x or y.

rayosborn · 2021-03-30T14:40:05Z

The comment by @mkoennecke about deprecating polar_angle, etc, alarmed me. Are people actually suggesting deprecating distance, polar_angle, azimuthal_angle, etc, in favor of abstract depends_on chains? I hope not because we use them all the time. Personally, I have never used transformations and I think it would be a mistake to force everyone to use them. It is another non-intuitive aspect of the standard for people to learn, and I don't think we even provide a tutorial (please point me to one if I'm wrong). If I have got the wrong end of the stick, please ignore this comment.

prjemian · 2021-03-30T18:23:45Z

As far as I know, there is no tutorial but there is documentation in addition to what is provided by the NXtransformation base class. The manual has a section with one example. More examples would be very useful.

yayahjb · 2021-03-30T19:57:10Z

I think Aaron is right that we should put most of this aside for the moment and talk it through slowly and carefully.

…

On Tue, Mar 30, 2021 at 10:40 AM Ray Osborn ***@***.***> wrote: The comment by @mkoennecke <https://github.com/mkoennecke> about deprecating polar_angle, etc, alarmed me. Are people actually suggesting deprecating distance, polar_angle, azimuthal_angle, etc, in favor of abstract depends_on chains? I hope not because we use them all the time. Personally, I have never used transformations and I think it would be a mistake to force everyone to use them. It is another non-intuitive aspect of the standard for people to learn, and I don't think we even provide a tutorial (please point me to one if I'm wrong). If I have got the wrong end of the stick, please ignore this comment. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#897 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB6EANHBBOWB242PRJGRP3TGHPFPANCNFSM4ZU3CGUQ> .

mkoennecke · 2022-06-13T13:47:00Z

I have updated this pull request by merging in aaron's changes which remove most of the deprecated messages and merged with main. Please review again.

prjemian

An apparent duplicate PR exists (#999 (comment)). Please consider which if these two pull requests to pursue and close the other with a suitable comment.

phyy-nx · 2022-06-14T22:36:33Z

I am working on a unified PR for #897 and #999

mkoennecke added 2 commits March 23, 2021 09:35

Intermediate step in the process of having each beam line component h…

3bf0bb3

…ave a depends_on field and NXtransformations group. Done until NXinsertion_device in the base class list.

Completed adding NXtransformations, depends_on and NXoff_geometry to …

2363fe1

…all base classes

mkoennecke requested review from PeterC-DLS, benajamin, prjemian, matthew-d-jones and yayahjb March 23, 2021 10:04

Trying to fix NXDL validation problems...

241c020

PeterC-DLS requested changes Mar 23, 2021

View reviewed changes

matthew-d-jones reviewed Mar 23, 2021

View reviewed changes

base_classes/NXaperture.nxdl.xml Outdated Show resolved Hide resolved

matthew-d-jones reviewed Mar 23, 2021

View reviewed changes

prjemian changed the title ~~Issue 681~~ add depends_on and related to all component type classes Mar 23, 2021

prjemian added this to the NXDL 2021.10 milestone Mar 23, 2021

prjemian added the code camp label Mar 23, 2021

Applied requested changes from the first round of comments

86e5b18

phyy-nx reviewed Mar 23, 2021

View reviewed changes

PeterC-DLS reviewed Mar 23, 2021

View reviewed changes

PeterC-DLS reviewed Mar 24, 2021

View reviewed changes

yayahjb reviewed Mar 24, 2021

View reviewed changes

Revert many of the deprectaions of fields related to positioning beam…

4d83038

…line components

phyy-nx added 3 commits March 31, 2021 09:56

For new transformation groups, don't specify a required name

5f531a9

Revert addition of new fields to NXtranformations

2535d04

Whitespace

7ec97c5

mkoennecke added 2 commits June 13, 2022 15:27

Merge branch 'main' into issue-681

a1be3a5

Merge branch 'issue-681-aaron' into issue-681

f45d91e

prjemian mentioned this pull request Jun 13, 2022

Adding depends on #999

Closed

prjemian requested changes Jun 14, 2022

View reviewed changes

benajamin mentioned this pull request Jun 14, 2022

NXDL: Can an attribute have an enumeration? #900

Closed

phyy-nx mentioned this pull request Jun 15, 2022

Add depends_on and NXtransformations to more base classes #1091

Merged

prjemian added the duplicate label Jun 15, 2022

yayahjb closed this in #1091 Jun 16, 2022

prjemian deleted the issue-681 branch June 23, 2022 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add depends_on and related to all component type classes #897

add depends_on and related to all component type classes #897

mkoennecke commented Mar 23, 2021 •

edited by prjemian

Loading

PeterC-DLS Mar 23, 2021

mkoennecke Mar 23, 2021

matthew-d-jones Mar 23, 2021

phyy-nx Mar 23, 2021

prjemian Mar 23, 2021

yayahjb Mar 24, 2021

matthew-d-jones Mar 23, 2021

matthew-d-jones Mar 23, 2021

mkoennecke Mar 23, 2021

matthew-d-jones Mar 23, 2021

phyy-nx Mar 23, 2021

PeterC-DLS Mar 23, 2021

PeterC-DLS Mar 23, 2021

PeterC-DLS Mar 24, 2021

yayahjb left a comment

phyy-nx commented Mar 29, 2021

yayahjb commented Mar 30, 2021 via email

mkoennecke commented Mar 30, 2021

rayosborn commented Mar 30, 2021

prjemian commented Mar 30, 2021

yayahjb commented Mar 30, 2021 via email

mkoennecke commented Jun 13, 2022

prjemian left a comment

phyy-nx commented Jun 14, 2022

add depends_on and related to all component type classes #897

add depends_on and related to all component type classes #897

Conversation

mkoennecke commented Mar 23, 2021 • edited by prjemian Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yayahjb left a comment

Choose a reason for hiding this comment

phyy-nx commented Mar 29, 2021

yayahjb commented Mar 30, 2021 via email

mkoennecke commented Mar 30, 2021

rayosborn commented Mar 30, 2021

prjemian commented Mar 30, 2021

yayahjb commented Mar 30, 2021 via email

mkoennecke commented Jun 13, 2022

prjemian left a comment

Choose a reason for hiding this comment

phyy-nx commented Jun 14, 2022

mkoennecke commented Mar 23, 2021 •

edited by prjemian

Loading