Proper inheritance of fields and attributes #621
Conversation
| if not set(NEXUS_TO_PYTHON_DATA_TYPES[self.dtype]).issubset(NEXUS_TO_PYTHON_DATA_TYPES[elem_type]): | ||
| return False |
There was a problem hiding this comment.
This is for allowing specialization of data types in inheritance, right? Is this something we want? Does NIAC have an opinion here?
There was a problem hiding this comment.
It's basically checking that the inherited concept does not have a mismatch with the concept its supposedly specializing. You need that so that e.g. a field mode(NX_CHAR) in a specialized NXdata doesn't get assigned AXISNAME in its inheritance.
There was a problem hiding this comment.
You could also check that they are equal, then e.g. NX_FLOAT in an appdef would not match NX_NUMBER in the base class. Not sure what we and/or NIAC want here.
There was a problem hiding this comment.
What I always understood it is okay to narrow down, e.g. make an NX_INT from the base class and NX_POSINT in the application definition (hence the subset here). None of this is really thought out though, most people in NIAC didn't really consider this inheritance on the fields level at all so far (see also discussion in nexusformat/definitions#1533), so we can more or less make our own conventions.
| if self.items is None: | ||
| return True |
There was a problem hiding this comment.
This breaks: AttributeError: 'NexusChoice' object has no attribute 'items'
Also, do we want to allow an inherited concept to completely remove any enumeration?
There was a problem hiding this comment.
This happens on ELECTRON_DETECTOR/pixel_shape.
Rather than showing the name, it is recognized also only as NexusChoise object in the debugger:
Not sure why
There was a problem hiding this comment.
Ah,I think because choice might not be implemented yet completely?
These don't even show up in the documentation page, see
https://manual.nexusformat.org/classes/base_classes/NXdetector.html
vs.
https://github.com/nexusformat/definitions/blob/2aede967caebc84f3335d50f1344f9b3ed2e8603/base_classes/NXdetector.nxdl.xml#L908
There was a problem hiding this comment.
Ah,I think because choice might not be implemented yet completely? These don't even show up in the documentation page, see manual.nexusformat.org/classes/base_classes/NXdetector.html vs. nexusformat/definitions@
2aede96/base_classes/NXdetector.nxdl.xml#L908
Yeah, docs are only built after NIAC releases, so we sort of have to go with our version to see the latest.
There was a problem hiding this comment.
NexusChoice isn't really working yet though, here it should really only check for NeXusEntity objects.
There was a problem hiding this comment.
That was my thought as well
There was a problem hiding this comment.
I moved the compatibility check to NexusEntity now and used the old inheritance for NexusChoice.
| # Should we really be this strict here? Or can appdefs define additional terms? | ||
| return False |
There was a problem hiding this comment.
Maybe needs discussion. I would say the base classes should contain the possible set of values, and the appdefs restrict them, but I'm not sure how it is handled by NIAC
There was a problem hiding this comment.
From my understanding, from the intital NeXus design, you can also add more enum items if needed, but this really breaks the way typical inheritance works, right?
There was a problem hiding this comment.
I think we don't have to be too pedantic here, but we should maybe think about if there are cases where this could lead to wrong inheritance assignments. I don't think so, though.
There was a problem hiding this comment.
We can also skip the items and dimensions checks for now and only re-activate if we ever see such problems.
There was a problem hiding this comment.
I disabled now that we check that the node's items are a subset of the items of the inherited concept.
One potentially misleading case is NXbeam/TRANSFORMATIONS/DIRECTION-field, which has transformation_type="translation".
In NXxps, we have NXxps/ENTRY/INSTRUMENT/beam_probe/transformations/beam_azimuth_angle-field, which has transformation_type="translation".
Without this check here, beam_azimuth_angle will inherit from DIRECTION because the type and unit are the same. This is however rather a problem of NXbeam, which should just not define a DIRECTION field with nameType="any" IMO.
| return False | ||
| return True | ||
|
|
||
| def _check_dimensions_fit(xml_elem: ET._Element) -> bool: |
There was a problem hiding this comment.
Do we want this/is this necessary?
There was a problem hiding this comment.
I deactivated this for now, we can see if any dimensions-related problems in the inheritance arise in the future.
b23f05a to
6f98325
Compare
…es with the nx_class
f96fa75 to
e0a70cd
Compare
.github/workflows/plugin_test.yaml
Outdated
| @@ -33,13 +33,13 @@ jobs: | ||
| branch: main | ||
| tests_to_run: tests/. | ||
| - plugin: pynxtools-mpes | ||
| branch: main | ||
| branch: entry-identifier |
There was a problem hiding this comment.
revert here and for raman/spm before merging
|
@rettigl this should work now. You should also be able to write This will log a warning if So currently, you either need to use |
There was a problem hiding this comment.
Thanks, this mostly seems to work properly now.
Checking of required attributes (@energy_reference etc. in NXmpes_arpes) did not work for me before, for which I added another fix.
This currently requires the notation @AXISNAME_indices[@energy_indices], but I think this is what we agreed on.
Maybe we can add a test for this as well, and maybe we should add some description of this to the documentation.
Otherwise, LGTM
No description provided.