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

Adding depends on #999

Closed
wants to merge 4 commits into from
Closed

Adding depends on #999

wants to merge 4 commits into from

Conversation

zjttoefs
Copy link
Contributor

@zjttoefs zjttoefs commented Mar 2, 2022

The depends_on way of defining the location of an instrument component works well. But in some cases the proper reference location that is placed by the depends_on is not well defined or obvious.

This pull request mainly aims to rectify this.

depends_on has been added to all base classes that would benefit from a well defined location. (NXnote would not, for example.)

In cases where we felt we could define the reference location, we have done so. With drawings in most cases.
Some base classes call for expert input what the reference location to place would be. TODO markers have been added.
If the preference would be to leave those classes untouched, that would mask an issue, but be acceptable to us.

OFF geometry shapes can be used to make the location unambiguous. For that reason they have been added. To keep the changeset small those edits could be deferred. Same for the explicit addition of NXtransformations.

In addition NXgeometry has been declared deprecated. Again, if this is not desired or does not reflect reality this does not have to happen as part of this PR.

The addition of the depends_on locations is a valuable tie breaker, though, and I would be thankful if that could be considered carefully.

zjttoefs added 2 commits March 2, 2022 18:29
* done this where possible (with reference drawings)
* added some more geometry related information
* deprecated old-style geometry
* minor adjustments
@zjttoefs zjttoefs added definitions NIAC should review The NIAC should review/discuss labels Mar 2, 2022
@zjttoefs
Copy link
Contributor Author

zjttoefs commented Apr 1, 2022

According to https://www.nexusformat.org/NIAC2022_spring.html#minutes and nexusformat/NIAC#109 (comment) this is approved.
How/when are we doing the merge?

@prjemian
Copy link
Contributor

Is this a duplicate of #897?

@benajamin
Copy link
Contributor

Is this a duplicate of #897?

Yes, this does appear to be a duplicate, but with some cross-reference links and additional image files that #897 could profit from appropriating.

@phyy-nx
Copy link
Contributor

phyy-nx commented Jun 14, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants