-
Notifications
You must be signed in to change notification settings - Fork 66
Update NXprocess documentation #1559
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
Update NXprocess documentation #1559
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the initiative lgtm, one important suggestion that can improve this pr
|
Can you comment on how this relates to https://manual.nexusformat.org/applying-nexus.html#processed-data? It seems to me that the original usage was to place one or more |
|
In fact, the prescrition of having separate entry foreach data processing step also suggests that if you want to specify restrictions/requirement on those steps, one must create seoarate Application definition for each of them. |
Seems overly restrictive and should be relaxed! |
mkuehbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am okay with this PR getting merged before #1422 but then this branch needs to be updated with main first.
|
telco today @phyy-nx: @rayosborn please edit this paragraph and add to this PR: |
|
In the above commit, I have suggested changes to the manual, which better reflect the ways in which the NXprocess group could be used. One thing that we didn't discuss just now in the Telco is how the NXdata group produced by the NXprocess is identified if it is not stored within the NXprocess group. There is no field or link within the NXprocess group that specifies which NXdata group was generated. In the NXtomoproc application definition, the only way to tell that the NXdata group is the result of the action described in the NXprocess group is in the tag descriptions, which are, of course, not stored in the file itself. In this case, there is only one NXdata group, but there could be more. I don't want to delay this PR any further, but I think in the future, we should consider making a recommendation that, if an application definition wants the NXdata group to be outside the NXprocess group, there should either be a link to the NXdata group within the NXprocess group (or vice versa) or that the path to the NXdata group should be stored as a field in the NXprocess group. |
mkuehbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebase again nexusformat/definitions.git main branch. Other than this, approved from my side.
PeterC-DLS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@rayosborn this PR is ready, @phyy-nx to be voted upon. Needs to be rebased against main before merge. Vote can start like discussed at the last telco, including #1422 |
|
Hi folks, as discussed on the Telco, this PR can now me moved to an online vote. Please vote using emojis (👍 for yes, 👎 for now, anything else e.g. 👀 for abstain). Voting will close in 2 weeks. Thanks! |
|
@PeterC-DLS, you suggested the possibility of adding some text about adding a "default" attribute to the NeXus manual text, but I decided in the end that it made more sense to add the attribute explicitly to the NXprocess group itself. Please could you confirm that my change to the PR is syntactically correct as well as appropriate. There have already been a few votes concerning this PR, so I will revert the change if anyone thinks it has been added too late. |
|
The vote has passed, thank you. @mkuehbach has requested a rebase, then it can be merged |
These are already allowed through extension of the NXobject group, but it is helpful to show that they have a particular purpose within this base class.
In principle, there could be more than one.
5b4fb4c to
75647df
Compare
|
The rebase has been completed. |
mkuehbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments were addressed appropriately as discussed at 2025/07/07 telco
A NIAC Telco discussion suggested that it was common practice to store the results of data analysis within the NXprocess group describing the operation. On reflection, I believe that this is a very valuable way of storing processed data, although I haven't used NXprocess groups in this way before. There are indeed many advantages to encapsulating both the description of the process (program name, data, input parameters) with the resulting data.
This PR does not change anything substantively. It modifies the base class documentation to make this usage more evident to non-specialists and explicity adds NXparameter and NXdata groups to store input parameters and the resulting data. Strictly speaking, both these group additions are redundant because their base classes have been added to the NXobject base class and are therefore already allowed. However, I think it is useful to add them here to show typical ways in which NXprocess groups are structured. As with groups in all base classes, their actual inclusion in a NeXus file is optional, unless an application definition explicitly requires them.