Skip to content

Generate example data files from NXDL specifications - #18

Merged
RussBerg merged 13 commits into
masterfrom
issue_17
Oct 29, 2020
Merged

Generate example data files from NXDL specifications#18
RussBerg merged 13 commits into
masterfrom
issue_17

Conversation

@RussBerg

@RussBerg RussBerg commented Jan 29, 2020

Copy link
Copy Markdown
Contributor

Generates example NeXus data files from the NXDL specifications, per #17.

Note this is a first attempt, these generated files do not pass cnxvalidate yet as more work needs to be done

note this is a first attempt, these generated files do not pass cnxvalidate yet as more work needs to be done
@phyy-nx

phyy-nx commented Jan 30, 2020

Copy link
Copy Markdown
Contributor

Hi, maybe a README file in the new nxdl folder? It could describe the nxdl_to_hdf5.py program and method used to generate the files, and even the current state of validation.

@prjemian

Copy link
Copy Markdown
Contributor

good

@RussBerg

RussBerg commented Jan 31, 2020

Copy link
Copy Markdown
Contributor Author

yep will do, I am on holidays for a week so I will address this when I am back.

@prjemian prjemian changed the title to address issue 17 Generate example data files from NXDL specifications Feb 14, 2020
@RussBerg

RussBerg commented Jul 3, 2020

Copy link
Copy Markdown
Contributor Author

ok I finally got around to getting this code to work, I have added some minor documentation and output from my code as well as the output from nxvalidate on each file. I am no longer generating the definitions for base_classes so only doing definitions in applications and contributed_definitions, however only definitions that contain an NXentry will be generated (in contributed_definitions).

The only real issue with generating the files is the link target syntax, nxvalidate will complain if it isnt exactly how it appears and the targets that are specified in the definition files are meant to be human readable guides as opposed to exact (or is that an invalid assumption?), the simplest would be to have the link target use the exact naming specified in its definition but I'm not sure how much resistance to that there might be to requiring definitions to do this.
Anyway any comments appreciated. @prjemian @phyy-nx

@RussBerg
RussBerg marked this pull request as draft October 19, 2020 21:13
@RussBerg RussBerg linked an issue Oct 19, 2020 that may be closed by this pull request
these changes do not fully support the generation of data specified with a rank that uses an expression it prints a Note to let the user know, and also doesn't die when it  comes across the expression
@RussBerg
RussBerg marked this pull request as ready for review October 20, 2020 16:22
@phyy-nx

phyy-nx commented Oct 20, 2020

Copy link
Copy Markdown
Contributor

Feedback from code camp:

  • Rename groups to untitled_groupname
  • Include autogenerated files, but note the specific nexusformat/definitions release used to generate them.
  • Possible folder name for the autogenerated files: autogenerated_examples (in the top level for repo). Code and readme stays in nxdl folder. (Move python script into nxdl folder)
  • Stretch goal (probably after code camp): add to nexusformat/definitions CI builds: generate nxdl h5 example files and validate using cnxvalidate.
  • Add doc strings as attributes.
  • Add readme field at the top. Include url back to example data. Also note the non-NeXus decorations that serve to help the user, such as required=True and the addition of the doc strings.

 - rename groups  to untitled_groupname
-  Include autogenerated files, but note the specific nexusformat/definitions release used to generate them.
- Add doc strings as attributes.
- Add readme field at the top. Include url back to example data. Also note the non-NeXus decorations that serve to help the user, such as required=True and the addition of the doc strings.
@RussBerg

Copy link
Copy Markdown
Contributor Author

Following a review the following changes were requested:

  • rename NX_class in attributes to 'type' for data types
  • add a prefix to the attributes that are from auto generation (required, doc)
  • get the latest release tag from git for version of the definitions to be used in README

@RussBerg
RussBerg marked this pull request as draft October 20, 2020 23:00
@RussBerg

Copy link
Copy Markdown
Contributor Author

I looked more into the grabbing what the current release of the definitions is for the local repo (to be used in the README data field of the generated file) and it looks like to me that the file NXDL_VERSION in the main directory of definitions does indicate the correct release tag of the repo, when I checkout different tags the contents of this file track correctly. The file contains a single string which is the release tag, @phyy-nx you were getting a different result yesterday so I am just wondering if I am missing something.

@prjemian

prjemian commented Oct 21, 2020 via email

Copy link
Copy Markdown
Contributor

@RussBerg

Copy link
Copy Markdown
Contributor Author

ok then I will continue to use the NXDL_VERSION in the local repo for the version tag

@RussBerg
RussBerg marked this pull request as ready for review October 21, 2020 16:58
     - rename NX_class in attributes to 'type' for data types
     - add a prefix (EX_) to the attributes that are from auto generation (required, doc)
     - get the latest release tag from git for version of the definitions to be used in README (no change was required)
@phyy-nx

phyy-nx commented Oct 21, 2020

Copy link
Copy Markdown
Contributor

I looked more into the grabbing what the current release of the definitions is for the local repo (to be used in the README data field of the generated file) and it looks like to me that the file NXDL_VERSION in the main directory of definitions does indicate the correct release tag of the repo, when I checkout different tags the contents of this file track correctly. The file contains a single string which is the release tag, @phyy-nx you were getting a different result yesterday so I am just wondering if I am missing something.

When I run the new code, the readme says v2020.1, but I'd expect it to say something like 'latest', 'master', or something, because the definitions have changed a lot since v2020.1. It's a minor issue though. I'd say this is fine as is.

@benajamin benajamin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is an enormous improvement on the current state of NeXus examples. One thing that I would like added (but merging is not dependent on), is to properly add the @axes and @{axis}_indices attributes on the NXdata group that describe the dimension scales of the plotable dataset(s). NXstxm had none, while NXcanSAS had lots of appropriate attributes, but without the correct values. It might take some work on the AppDef nxdl files to get this working properly and I would prefer to get the current version merged and then make a new issue to later/next work on improving the NXdata groups.

@prjemian

Copy link
Copy Markdown
Contributor

Most common ERROR flagged by punx validate is missing /entry described by a @default attribute. Such as in NXarpes.hdf5:/@default="/entry" but there is no matching NXarpes.hdf5:/entry. This is an error, the value described by the default attribute must exist. The correct value for this data file would be: NXarpes.hdf5:/@default="/untitled_entry".

12 instances of not found occur in the output (review.txt below) from running this script in the NeXus/exampledata/autogenerated_examples/nxdl/applications directory (on linux):

#!/bin/bash

# ./punx_review.sh 2>&1 | tee /tmp/review.txt

for i in $(ls NX*.hdf5); do
    echo $i
    punx val $i
    punx tree $i
    echo "------------------"
done

output: review.txt


Q: What are the NXentry groups named?
A: this is a search: grep :NXentry /tmp/review.txt

(bluesky_2020_9) prjemian@poof ~/.../nxdl/applications $ grep :NXentry /tmp/review.txt 
  entry:NXentry
  untitled_entry:NXentry
  untitled_entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  untitled_entry:NXentry
  entry:NXentry
  entry:NXentry
  untitled_entry:NXentry
  entry:NXentry
  entry:NXentry
  untitled_entry:NXentry
  untitled_entry:NXentry
  untitled_entry:NXentry
  untitled_entry:NXentry
  untitled_entry:NXentry
  untitled_entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  untitled_entry:NXentry
  untitled_entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry
  entry:NXentry

@prjemian prjemian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Problems as noted with some /@default values (named group does not exist).

@RussBerg

Copy link
Copy Markdown
Contributor Author

nice catch @prjemian !

@RussBerg
RussBerg requested a review from prjemian October 29, 2020 17:49

@prjemian prjemian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All HDF5 files pass NeXus validation with punx (note that punx does not validate application definitions yet so this is not the deepest analysis possible).

@RussBerg
RussBerg merged commit cbedd8d into master Oct 29, 2020
@RussBerg
RussBerg deleted the issue_17 branch October 29, 2020 18:29
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.

from code camp: add generated nxdl code and files

4 participants