-
Notifications
You must be signed in to change notification settings - Fork 56
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
NXdata: improve documentation regarding axes #1213
NXdata: improve documentation regarding axes #1213
Conversation
f065035
to
2359c3b
Compare
571420f
to
234cd6d
Compare
8177df7
to
b99c398
Compare
bfa0832
to
73f59d8
Compare
8539f58
to
c02e0c2
Compare
Changes:
|
For reference, I took @rayosborn's description of axes #1213 (comment)
and merged it with my original wording to obtain this
I mainly had an issue with this
This seems to imply the AXISNAME_indices are to be ignored when you only have rank 1 axes. This is logically very hard to implement mostly for HDF5 readers. In the merged version this becomes
This is logically much simpler. @rayosborn's description in spirit wants to say that most of our NXdata groups do not has AXISNAME_indices. So I added this paragraph
|
To accommodate @phyy-nx who prefer positive wording I replaced
with
|
I did the splitting in another way with subsections rank and dimensions. The axes section should be easier to read now. |
Code review:
|
Applied the requested modifications from #1213 (comment): examples are merged with the doc sections and adapted to match those sections. |
0b63ab1
to
b981fa5
Compare
b981fa5
to
d585b48
Compare
From telco, let's get the same subcommittee mentioned above to re-review and approve the PR (@phyy-nx @woutdenolf @rayosborn @sanbrock). Further reviews are welcome. Target is November 2nd. Thanks! |
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.
Looks like a big improvement to me!
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
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! Looks good!
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.
Great job.
Ok! Based on Telcos and these approvals, @woutdenolf feel free to merge at your convenience. Thanks! |
Thanks for the review everyone! |
Closes #1212
In this PR we refactor the NXdata definition to resolve inconsistencies in definition of
@axes
,AXISNAME_indices
andAXISNAME
. NIAC discussions:The proposed changes do not change anything to the current NXdata, they just clear up ambiguity:
@AXISNAME_indices
: data dimension(s) spanned byAXISNAME
. When missing it defaults to the index or indices ofAXISNAME
in@axes
.@AXISNAME_indices
: a single integer or an array of integers.AXISNAME
must be equal to the number of dimensions it spansdimension scales
is a confusing term which comes from HDF5 terminology: useaxes
,axis coordinates
orindependent variables
depending on the contextdescription
description
in sub-sections: signals, axes, uncertainties and examplesProper code examples with be added in a separate merge request: #1253
A python reader would have to do something like this to figure out what all the axes are and to which data dimensions they belong
Rendering of the PR
https://hdf5.gitlab-pages.esrf.fr/nexus/nxdata_axes/classes/base_classes/NXdata.html