Skip to content

Support for appdef extends #339

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

Merged
merged 8 commits into from
Jun 14, 2024
Merged

Support for appdef extends #339

merged 8 commits into from
Jun 14, 2024

Conversation

domna
Copy link
Collaborator

@domna domna commented Jun 4, 2024

This adds support for the extends keyword for application definitions.

TODO:

  • Add test case with NXtest using the extends keyword and add tests for it
  • Fix NXellipsometry problems
  • Check NXxps
  • Check NXmpes_arpes

@coveralls
Copy link

coveralls commented Jun 4, 2024

Pull Request Test Coverage Report for Build 9418779202

Details

  • 28 of 33 (84.85%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.7%) to 96.916%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/dataconverter/test_nexus_tree.py 28 33 84.85%
Totals Coverage Status
Change from base Build 9416111276: -0.7%
Covered Lines: 597
Relevant Lines: 616

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9364352509

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.008%) to 78.47%

Files with Coverage Reduction New Missed Lines %
pynxtools/dataconverter/validation.py 1 68.2%
pynxtools/dataconverter/helpers.py 1 79.21%
Totals Coverage Status
Change from base Build 9347831818: 0.008%
Covered Lines: 2821
Relevant Lines: 3595

💛 - Coveralls

@domna
Copy link
Collaborator Author

domna commented Jun 7, 2024

@rettigl The phoibos example now runs through without a warning when using NXmpes_arpes.
I needed to make the following changes to the config file:

diff --git a/specsscan/config/NXmpes_arpes_config.json b/specsscan/config/NXmpes_arpes_config.json
index edf8162..0ae5b3c 100755
--- a/specsscan/config/NXmpes_arpes_config.json
+++ b/specsscan/config/NXmpes_arpes_config.json
@@ -1,7 +1,7 @@
 {
   "/@default": "entry",
   "/ENTRY[entry]/@default": "data",
-  "/ENTRY[entry]/definition": "NXmpes",
+  "/ENTRY[entry]/definition": "NXmpes_arpes",
   "/ENTRY[entry]/definition/@version": "None",
   "/ENTRY[entry]/title": "@attrs:metadata/entry_title",
   "/ENTRY[entry]/start_time": "@attrs:metadata/timing/acquisition_start",
@@ -135,7 +135,7 @@
     "diameter": 300.0,
     "diameter/@units": "mm",
     "entrance_slit": {
-      "shape": "curved",
+      "shape": "curved slit",
       "size": 1.0,
       "size/@units": "mm"
     },
@@ -337,13 +337,13 @@
   },
   "/ENTRY[entry]/data": {
     "@axes": "@data:dims",
-    "AXISNAME_indices[@*_indices]": "@data:*.index",
+    "@*_indices": "@data:*.index",
     "@signal": "data",
     "data": "@data:data",
     "data/@units": "counts",
-    "AXISNAME[*]": "@data:*.data",
-    "AXISNAME[*]/@units": "@data:*.unit",
-    "AXISNAME_depends[@*_depends]": "@attrs:metadata/scan_info/coordinate_depends/*",
+    "*": "@data:*.data",
+    "*/@units": "@data:*.unit",
+    "@*_depends": "@attrs:metadata/scan_info/coordinate_depends/*",
     "energy/@type": "kinetic"
   }
 }

I think it all makes sense except the change of AXISNAME might not be that obvious. While using the upper case notation for nexus groups I think it does not make much sense to apply this concept to field namefitting. For a group we would actually write an NX_class attribute to the file but for fields we have no such concept and we have to completely rely on the namefitting concept for reading the file. Therefore, I think it makes sense to avoid giving information which will be lost while writing and validating the file. We could however think to add an additional check which checks that all the fields written in AXISNAME are actually in the @axes array (which actually associates them to AXISNAME), but this will be something very special to NXdata and might be confusing in other places. Anyways, without using AXISNAME this works fine.

@domna
Copy link
Collaborator Author

domna commented Jun 7, 2024

I also created a PR here OpenCOMPES/specsanalyzer#44 to bring the changes to specsanalyzer

Copy link
Collaborator

@lukaspie lukaspie left a comment

Choose a reason for hiding this comment

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

LGTM, though I don't fully understand what the code is doing. I checked with NXmpes_arpes as outlined above and it works. Should we maybe add a test case for an extending app def?

@domna domna force-pushed the support-extends-in-nexus-tree branch from 520cb2c to f2dfd9f Compare June 7, 2024 11:42
@domna
Copy link
Collaborator Author

domna commented Jun 7, 2024

LGTM, though I don't fully understand what the code is doing. I checked with NXmpes_arpes as outlined above and it works. Should we maybe add a test case for an extending app def?

Yes, I think it makes sense to make an extends case for NXtest. I'll add it. Also there are still some problems in NXellipsometry. It's probably due to the validation itself and not the extends but I would bring the fixes here, too.

@domna domna mentioned this pull request Jun 7, 2024
1 task
@domna
Copy link
Collaborator Author

domna commented Jun 7, 2024

@lukaspie, this is what I get when I run the xps examples with NXxps

I worked on this branch FAIRmat-NFDI/nexus_definitions#170, and pynx-xps main (added NXxps to the supported readers there, though).

For the sle example

Using xps reader to convert the given files:  
• EX439_S718_Au.sle
• eln_data_sle.yaml
The required group, /ENTRY[Au_in_25mBar_O2__Au4f]/geometries, hasn't been supplied.
The required group, /ENTRY[Au_in_25mBar_O2__Au4f]/INSTRUMENT[instrument]/ELECTRONANALYSER[electronanalyser]/transformations, hasn't been supplied.
The required group, /ENTRY[Au_in_25mBar_O2__Au4f]/SAMPLE[sample]/transformations, hasn't been supplied.
The required group, /ENTRY[Au_in_vacuum__Au4f]/geometries, hasn't been supplied.
The required group, /ENTRY[Au_in_vacuum__Au4f]/INSTRUMENT[instrument]/ELECTRONANALYSER[electronanalyser]/transformations, hasn't been supplied.
The required group, /ENTRY[Au_in_vacuum__Au4f]/SAMPLE[sample]/transformations, hasn't been supplied.
Field /ENTRY[Au_in_25mBar_O2__Au4f]/data/energy/@long_name written without documentation.
Field /ENTRY[Au_in_vacuum__Au4f]/data/energy/@long_name written without documentation.
The output file generated: Au_25_mbar_O2_no_align.nxs.

For the phi example (with --ignore-undocumented)

dataconverter --nxdl NXxps --reader xps --ignore-undocumented SnO2_10nm.spe eln_data_phi.yaml
Using xps reader to convert the given files:  
• SnO2_10nm.spe
• eln_data_phi.yaml
The required group, /ENTRY[Su1s]/geometries, hasn't been supplied.
The required group, /ENTRY[Su1s]/INSTRUMENT[instrument]/ELECTRONANALYSER[electronanalyser]/transformations, hasn't been supplied.
The required group, /ENTRY[Su1s]/SAMPLE[sample]/transformations, hasn't been supplied.
The output file generated: output.nxs.

Does it make sense since the example is not ready or does it seem to you that there is something up with the validation itself?

@lukaspie
Copy link
Collaborator

lukaspie commented Jun 7, 2024

@lukaspie, this is what I get when I run the xps examples with NXxps

I worked on this branch FAIRmat-NFDI/nexus_definitions#170, and pynx-xps main (added NXxps to the supported readers there, though).

For the sle example

Using xps reader to convert the given files:  
• EX439_S718_Au.sle
• eln_data_sle.yaml
The required group, /ENTRY[Au_in_25mBar_O2__Au4f]/geometries, hasn't been supplied.
The required group, /ENTRY[Au_in_25mBar_O2__Au4f]/INSTRUMENT[instrument]/ELECTRONANALYSER[electronanalyser]/transformations, hasn't been supplied.
The required group, /ENTRY[Au_in_25mBar_O2__Au4f]/SAMPLE[sample]/transformations, hasn't been supplied.
The required group, /ENTRY[Au_in_vacuum__Au4f]/geometries, hasn't been supplied.
The required group, /ENTRY[Au_in_vacuum__Au4f]/INSTRUMENT[instrument]/ELECTRONANALYSER[electronanalyser]/transformations, hasn't been supplied.
The required group, /ENTRY[Au_in_vacuum__Au4f]/SAMPLE[sample]/transformations, hasn't been supplied.
Field /ENTRY[Au_in_25mBar_O2__Au4f]/data/energy/@long_name written without documentation.
Field /ENTRY[Au_in_vacuum__Au4f]/data/energy/@long_name written without documentation.
The output file generated: Au_25_mbar_O2_no_align.nxs.

For the phi example (with --ignore-undocumented)

dataconverter --nxdl NXxps --reader xps --ignore-undocumented SnO2_10nm.spe eln_data_phi.yaml
Using xps reader to convert the given files:  
• SnO2_10nm.spe
• eln_data_phi.yaml
The required group, /ENTRY[Su1s]/geometries, hasn't been supplied.
The required group, /ENTRY[Su1s]/INSTRUMENT[instrument]/ELECTRONANALYSER[electronanalyser]/transformations, hasn't been supplied.
The required group, /ENTRY[Su1s]/SAMPLE[sample]/transformations, hasn't been supplied.
The output file generated: output.nxs.

Does it make sense since the example is not ready or does it seem to you that there is something up with the validation itself?

This is all fine, these examples currently don't populate the transformations. Good that the validation picks that up though, so should be fine. Actually, maybe NXxps is not yet a good test for this, I was probably thinking about validating this feature in a different way. I think it's okay to just test it on NXmpes_arpes and NXellips, NXxps does not add anything that is not also covered by these two.

@domna domna self-assigned this Jun 7, 2024
@domna domna merged commit 70d74b3 into master Jun 14, 2024
9 checks passed
@domna domna deleted the support-extends-in-nexus-tree branch June 14, 2024 13:16
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.

3 participants