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

improve description of the value of the default attribute #1022

Merged
merged 24 commits into from
Jun 14, 2022

Conversation

@prjemian prjemian added this to the NXDL 2022.03 milestone Mar 7, 2022
@prjemian prjemian self-assigned this Mar 7, 2022
@prjemian prjemian requested review from a team, sanbrock and woutdenolf March 7, 2022 22:55
@prjemian prjemian marked this pull request as ready for review March 7, 2022 22:59
@prjemian
Copy link
Contributor Author

prjemian commented Mar 7, 2022

Is there any other documentation to be revised?

@prjemian prjemian added the telco label Mar 8, 2022
Copy link
Contributor

@woutdenolf woutdenolf left a comment

Choose a reason for hiding this comment

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

LGTM. I added some comments related to keeping the docs HDF5 agnostic.

I also see you often make the distinction between a field and a link to field (e.g. either as a NeXus field or as a link to a NeXus field). I guess this is done all over the documentation but imo this is an HDF5 feature which has nothing to do with the NeXus standard. But anyway, that should be discussed elsewhere.

base_classes/NXdata.nxdl.xml Outdated Show resolved Hide resolved
base_classes/NXdata.nxdl.xml Outdated Show resolved Hide resolved
manual/source/datarules.rst Outdated Show resolved Hide resolved
@mkoennecke
Copy link
Contributor

I am in favor of woutdenolfs more general wording.

@prjemian
Copy link
Contributor Author

prjemian commented Jun 13, 2022

@benajamin notes that this legacy text is out of date:

* Each :ref:`NXdata` group will define only one data set
containing plottable data, dimension scales, and
possibly associated standard deviations.
Other data sets may be present in the group.

Look at this code (in base_classes/NXdata.nxdl.xml) for some suggestion:

   <attribute name="auxiliary_signals">
                   ``signal`` attribute).  Each auxiliary signal needs to be of

<attribute name="auxiliary_signals">
<doc>
.. index:: plotting
Array of strings holding the names of additional signals to
be plotted with the default signal (specified by the
``signal`` attribute). Each auxiliary signal needs to be of
the same shape as the default signal.
.. NIAC2018:
https://www.nexusformat.org/NIAC2018Minutes.html
</doc>
</attribute>

@prjemian
Copy link
Contributor Author

Also, NXentry no longer requires a NXdata group.

@benajamin
Copy link
Contributor

@woutdenolf

I also see you often make the distinction between a field and a link to field (e.g. either as a NeXus field or as a link to a NeXus field). I guess this is done all over the documentation but imo this is an HDF5 feature which has nothing to do with the NeXus standard. But anyway, that should be discussed elsewhere.

I agree in staying general, but I don't know if it is fair to say that only HDF5 has links.

@prjemian
Copy link
Contributor Author

@benajamin @woutdenolf This is ready for review again.

@benajamin benajamin assigned prjemian and unassigned prjemian Jun 14, 2022
Copy link
Contributor

@woutdenolf woutdenolf left a comment

Choose a reason for hiding this comment

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

Apart from some technical fixes, it seems all good now. Thanks for the hard work!

A small additional comment: I noticed you used the phrase

The value of the default attribute :ref:names <validItemName> an
existing child of this group

The link to validItemName is not really needed imo but I guess it doesn't hurt either.

manual/source/examples/epics/write_nexus_file.py Outdated Show resolved Hide resolved
manual/source/datarules.rst Show resolved Hide resolved
WARNING: Failed to create a cross reference. A title or caption not found: find-plottable-data-v3
Copy link
Contributor

@benajamin benajamin left a comment

Choose a reason for hiding this comment

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

LGTM

@prjemian
Copy link
Contributor Author

Thanks for the reviews!

@prjemian prjemian merged commit e045360 into main Jun 14, 2022
@prjemian prjemian deleted the 1003-default-attribute-value branch June 14, 2022 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@default is a step by step process instead of a direct jump (should be better documented)
4 participants