-
Notifications
You must be signed in to change notification settings - Fork 8
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
Bugs in NXmpes definition #230
Comments
The second part is addressed in PR #128, so only the former remains (probably pynxtools-related?) |
I think |
I don't really understand this error. There is no named field transformations in either NXmpes/NXinstrument/NXelectronanalyser or in the base class NXmpes. I guess you are working with |
Me neither. I just used the latest definitions installed by pynxtools. I will look into this when I find time. |
This bug must be somewhere in the validation code. However, I don't understand it well enough... |
The issue is somewhere here: So, it makes sense that the code complains (as lower_case transformations is not defined), but the error message is very unintuitive and misleading. |
If I try to use the inherited appdef NXmpes_arpes, things become really crazy:
To me it appears this validation code has not been thoroughly tested enough... |
Indeed that is a problem with the algorithm. It goes through all required fields and tries to find a namefit the available names and they are only considered if they are not in all the children names (here including all inherited ones). This means that it also can namefit a single name twice if it matches to multiple categories (including the consequences of missing fields one level down). For this we need to resolve the ambiguity by stating it with the class as you correctly found. I think we can improve the algorithm for this in the future but for the first basic version we need to resolve the ambiguity by stating the class. A return value of 0 for nexus_definitions/dev_tools/utils/nxdl_utils.py Lines 112 to 147 in ea6b7b2
I agree. Generally, all error messages and reporting for the new verification have room for improvement. I plan to do improvements with the cli #validation and hdf tree traversal: FAIRmat-NFDI/pynxtools#333
Indeed, we brought this quickly with only a small testing set on our examples. But the old algorithm was broken anyways, was slow and also did not verify everything (especially multiple groups belonging to the same concept) so we decided to bring it in already and then fix all the upcoming issues with the new algorithm. Your long list of problems, I believe, arise from the fact that |
@rettigl I started to work on supporting the extends keyword here: FAIRmat-NFDI/pynxtools#339 Which example did you use for the above tests? Then I'll use this for testing and debugging. |
This was the conversion of the phoibos scan to NXmpes_arpes, which I did on the central Nomad, with updated packages in the jupyterlab container |
Closed as handled by FAIRmat-NFDI/pynxtools#339 |
There seem to be several issues with both the NXmpes definitions, as well as with the parsing code in the latest version of pynxtools
This path should not exist, and does not occur in NXmpes, so definitely should not be required.
The text was updated successfully, but these errors were encountered: