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

Geometry docs #397 #554

Merged
merged 5 commits into from
Jun 21, 2017
Merged

Geometry docs #397 #554

merged 5 commits into from
Jun 21, 2017

Conversation

prjemian
Copy link
Contributor

@prjemian prjemian commented Apr 5, 2017

is this ready to merge?

@prjemian prjemian self-assigned this Apr 5, 2017
@prjemian
Copy link
Contributor Author

prjemian commented Jun 7, 2017

@PeterC-DLS : Is this ready to merge?

@PeterC-DLS
Copy link
Contributor

I guess so. It may be missing a discussion on how NeXus versus CIF.

@prjemian
Copy link
Contributor Author

prjemian commented Jun 9, 2017

@zjttoefs , @mkoennecke : you have been requested to review this PR.

remove extra or inconsistent comma
add explicit depends_on to end chain
Copy link
Contributor

@zjttoefs zjttoefs left a comment

Choose a reason for hiding this comment

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

I pushed two commits with the only comments I had.
In the examples all vectors are integers and that's consistent with the nxdl, so changed for NX_NUMBER as data type.
In the euler example I remove commas and added the (recommended) explicit end to the chain. I did not wrap the motion axis into NXtranformations. That might be nice, but would need some words as well.

Copy link
Contributor

@mkoennecke mkoennecke left a comment

Choose a reason for hiding this comment

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

This appears OK with me

@prjemian prjemian merged commit 7455d33 into master Jun 21, 2017
@prjemian prjemian deleted the geometry_docs_397 branch June 21, 2017 12:05
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