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

Make example NXreflection files more like base classes #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

phyy-nx
Copy link
Contributor

@phyy-nx phyy-nx commented Jan 30, 2020

Should be merged concurrently with nexusformat/definitions#752

Copy link
Contributor

@prjemian prjemian left a comment

Choose a reason for hiding this comment

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

No such base class: NXdials

      dials:NXdials
        @NX_class = NXdials

@prjemian
Copy link
Contributor

hang on, those comments are from viewing content in master

@prjemian
Copy link
Contributor

prjemian commented Jan 30, 2020

time to rename DETAILS.rst to README.txt. README is consistent with other directories. This is not restructured text format.

@prjemian
Copy link
Contributor

from punx

hdf5 address finding test name comment
/entry/experiment_0/dials ERROR known NXDL NXdials: recognized NXDL specification
/entry/experiment_0/dials ERROR NeXus base class unknown NeXus base class: NXdials
/entry/experiment_0/dials@NX_class ERROR attribute value not a recognized NXDL class: NXdials

@phyy-nx
Copy link
Contributor Author

phyy-nx commented Jan 30, 2020

Right, NXdials is something from a few years ago that didn't make the jump to NeXus itself. I think it's extra glue to help DIALS with NXreflections. I'm not sure if it is relevant anymore. I'll ask around in DIALS land but I don't think it matters for this PR.

@prjemian
Copy link
Contributor

punx is also telling me (but I'm not certain I can trust it for this to be an error yet):

hdf5 address finding test name comment
/entry/experiment_0@None ERROR NXDL field NXmx:start_time not found
/entry/experiment_0@None ERROR NXDL field NXmx:end_time not found

These fields are defined in the NXmx definition that appears in v2020.1rc2 which is used for validation. punx's validation of data files with application definitions needs serious work; this is an example.

@prjemian
Copy link
Contributor

Those are optional fields and punx does not yet understand about that possibility. Ignore that finding, then.

@prjemian
Copy link
Contributor

NXdials does matter. It is not allowed in a NeXus file. See the second bullet point after https://manual.nexusformat.org/datarules.html. Basically, you must only use the @NX_class attribute with the name of a NeXus base class, application definition, or contributed definition. NXdials is not in that list. You could change the name of the attribute to something that does not begin with NX (upper or lower case) and then NeXus would not care about it, it would be a group that NeXus would ignore. OR, you could find an existing base class...

@prjemian
Copy link
Contributor

my imagination suggests @possible_NX_class

@phyy-nx
Copy link
Contributor Author

phyy-nx commented Jan 30, 2020

Thanks @prjemian! I've created an issue here: dials/dials#1124.

Do you want to hold this PR though? Or create a separate issue about this? For me, the dials issue is sufficient.

@prjemian
Copy link
Contributor

I believe that a quick decision is not so likely. Are you OK with delaying?

@phyy-nx
Copy link
Contributor Author

phyy-nx commented Jan 31, 2020

Whatever it takes is fine, since isn't part of the NeXus release. I do think the NXdials thing and the intent of this pull request are being conflated. This pull request is just to sync with nexusformat/definitions#752.

@prjemian
Copy link
Contributor

The only problem is adding a data file with a non-compliant component like that tempts others to follow the practice. Can you change that for the deposition here, then revise it after consideration from dials/dials?

@phyy-nx
Copy link
Contributor Author

phyy-nx commented Feb 5, 2020

@prjemian @NX_class=NXdials now removed. Ready for re-review at your convenience. Thanks.

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.

2 participants