Add new geometry definitions #601
Conversation
|
|
||
| <dimensions rank="2"> | ||
| <dim index="1" value="i" /> | ||
| <dim index="2" value="2 or 3" /> |
There was a problem hiding this comment.
You cannot define a cylinder in two dimensions.
base_classes/NXdetector.nxdl.xml
Outdated
| Shape description of the whole detector. Use only if pixels in the detector are not of uniform shape. | ||
| </doc> | ||
| </group> | ||
| <group name="pixel_shape" type="NXcylindrical_geometry"> |
There was a problem hiding this comment.
You cannot have two "pixel_shape" groups with the same name. For now this could be an either/or choice, but in the long run we did discuss some scheme of providing alternatives (OFF for the dumb programs, cylinders for the slightly smarter ones, and the CAD thing for whoever wants to implement it - or so).
Probably worth thinking about that some more.
| if the pixel shapes are non-uniform, to describe the shape of the whole detector. | ||
| </doc> | ||
|
|
||
| <field name="vertices" type="NX_FLOAT" units="NX_LENGTH"> |
There was a problem hiding this comment.
I would allow NX_NUMBER. Then you could us mm and store INTs.
Applies a number of times.
base_classes/NXoff_geometry.nxdl.xml
Outdated
|
|
||
| <dimensions rank="2"> | ||
| <dim index="1" value="i" /> | ||
| <dim index="2" value="2 or 3" /> |
There was a problem hiding this comment.
I don't think we're saving much by allowing 2D here.
…e dimensions for vertex coords.
base_classes/NXdetector.nxdl.xml
Outdated
| subdivision of a detector. | ||
| </doc> | ||
| </group> | ||
| <group name="pixel_shape" type="NXoff_geometry"> |
There was a problem hiding this comment.
My gut feeling would be to wrap the two kinds of pixel_shape in an XSD (or whatever the correct thing is). That makes it obvious that you cannot have both.
To the very least I would group the two next to each other. Same for detector_shape.
There was a problem hiding this comment.
I believe @zjttoefs is correct. A definition with a this-or-that component needs support in the XSD (XML Schema for the NXDL). I'll take a deeper look later today. There may already be a precedent in another definition.
There was a problem hiding this comment.
The documentation builds with no errors.
There was a problem hiding this comment.
Is there a typo on this line? Perhaps, instead, this:
<dim index="2" value="3" />
There was a problem hiding this comment.
A matter of style in the documentation (in the NXDL files).
Good practice to keep the first documentation to one line and separate from any succeeding lines. This allows automatic generation of concise summaries such as for the base class definitions.
Also, please make a line break between 60-80 characters on a line so that the raw source code becomes much easier to read and review such as this example.
There was a problem hiding this comment.
Considering this line:
This dataset should only be used only if the ``NXoff_geometry`` group is describing a detector.
Can we describe the algorithm by which the XML Schema could enforce this? The XML Schema should have no knowledge of any of the NXDL classes. Probably best not to implement in the XSD since that would introduce this knowledge.
Could we enforce this by some rule in the NXDL file itself? Where is the information provided by which validation can check this? By traversing the parent (or chain of parents) of the NXoff_geometry instance?
There was a problem hiding this comment.
That the documentation builds is good, but I'm not sure it is sufficient. The validation may need some help to know that there can be one of two groups under a single name. Even just for the person reading the NXDL <choice> make things that little bit more obvious.
|
@matthew-d-jones if you could check it all works with the |
|
@mkoennecke volunteered to make |
|
I believe the best way to proceed for the proposed |
|
Once #619 is resolved, this PR can proceed. |
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://definition.nexusformat.org/nxdl/3.1 ../nxdl.xsd" | ||
| name="NXcylindrical_geometry" | ||
| version="1.0" |
| <!-- | ||
| # NeXus - Neutron and X-ray Common Data Format | ||
| # | ||
| # Copyright (C) 2008-2017 NeXus International Advisory Committee (NIAC) |
There was a problem hiding this comment.
change 2017 to 2018 in the copyright date
|
I will squash merge, then fix these minor things myself. |
Added the new geometry groups for describing the shape of any beamline component (including detectors) as discussed at the NeXus Code Camp (11/17).