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

Document concepts of the new geometry description #612

Conversation

matthew-d-jones
Copy link
Member

#601 should be merged before this PR

Documented concepts of the new shape description (NXoff_geometry and NXcylindrical_geometry).
See html/design.html#design-geometry section for changes.

@mkoennecke
Copy link
Contributor

I have looked at this again. To this purpose I merged the other branch with this. There are some rough edges in the writing, for example:

Conversion between OFF files and the NeXus description is straightforward allows geometry to be defined

I think there is a period and a few words missing there.

The next thing I stumbled over is the detector_faces array in NXoff_geometry. It took me a while to understand that l is just an ordering dimensions and that the 2 in the dimensions actually describes a pair of detectorID, faceID. IMHO, this needs to be documented more clearly. Another question: is this array sorted by detectorID? Such that I can expect to find all faces making up a detector volume in a consecutive order.

And I think the NIAC should vote on this

@matthew-d-jones
Copy link
Member Author

Thanks for the feedback @mkoennecke. I'll try to clarify these points.
Would NOBUGS be the next opportunity for a NIAC vote?

@mkoennecke
Copy link
Contributor

In principle NOBUGS would be the next point. But we could also vote by email on this. May be this is a topic for the next Telco?

@zjttoefs
Copy link
Contributor

I would say we have a sufficient mandate from the code camp to continue down this route. No need to accelerate a vote.
Presenting it at one of the next teclos for information sounds like a good idea, though.

@matthew-d-jones
Copy link
Member Author

I have tried to improve the wording in the couple of places @mkoennecke pointed out.

Sorting of the detector_faces is a good question. I've specified that it should be in ascending order of faces index, which is what my python scripts implement, but perhaps ordering by detector ID would be more useful to readers of the file...

@zjttoefs
Copy link
Contributor

I would suggest also presenting the NeXus equivalent to the cube.off file. That would explain the need for the winding order.

Detector Shape Descriptions
---------------------------

An ``NXoff_geometry`` or ``NXcylindrical_geometry`` group named ``shape`` can
Copy link

Choose a reason for hiding this comment

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

The field name added was detector_shape, not shape

@zjttoefs zjttoefs merged commit 5c5dbe6 into nexusformat:master Oct 27, 2018
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.

4 participants