8000 Fixes child retrieval for fields by domna · Pull Request #256 · FAIRmat-NFDI/pynxtools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fixes child retrieval for fields #256

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 23, 2024
Merged

Fixes child retrieval for fields #256

merged 3 commits into from
Feb 23, 2024

Conversation

domna
Copy link
Collaborator
@domna domna commented Feb 22, 2024

This fixes the child retrievel which resulted in an error for non-existing fields, because parts of the nxdl paths were removed

@domna domna requested a review from rettigl February 22, 2024 14:04
@domna domna self-assigned this Feb 22, 2024
@domna domna requested a review from sherjeelshabih February 22, 2024 14:04
@coveralls
Copy link
coveralls commented Feb 22, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8008938554

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 36 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.2%) to 46.328%
Files with Coverage Reduction New Missed Lines %
tests/dataconverter/test_writer.py 1 96.77%
pynxtools/dataconverter/readers/em_spctrscpy/utils/hspy/em_hspy_adf.py 2 11.38%
pynxtools/dataconverter/readers/em_spctrscpy/utils/hspy/em_hspy_eels.py 3 9.38%
pynxtools/dataconverter/readers/em_spctrscpy/utils/hspy/em_hspy_xray.py 4 8.59%
pynxtools/dataconverter/helpers.py 12 91.61%
pynxtools/dataconverter/convert.py 14 80.71%
Totals Coverage Status
Change from base Build 8002598971: -0.2%
Covered Lines: 3886
Relevant Lines: 8388
💛 - Coveralls

sherjeelshabih
sherjeelshabih reviewed Feb 22, 2024
View reviewed changes
@rettigl
Copy link
Collaborator
rettigl commented Feb 22, 2024

This fix solves the original issue (Exception being raised), but does not seem to do the right thing either. With this fix, I get these error reports from the convert function:

The data entry corresponding to /ENTRY[entry]/@version is required and hasn't been supplied by the reader.
The data entry corresponding to /ENTRY[entry]/geometries/arpes_geometry/TRANSFORMATIONS[transformations] is required and hasn't been supplied by the reader.
The data entry corresponding to /ENTRY[entry]/INSTRUMENT[instrument]/ELECTRONANALYSER[electronanalyser]/transformations/@vector is required and hasn't been supplied by the reader.
The data entry corresponding to /ENTRY[entry]/INSTRUMENT[instrument]/ELECTRONANALYSER[electronanalyser]/transformations/@transformation_type is required and hasn't been supplied by the reader.
The data entry corresponding to /ENTRY[entry]/INSTRUMENT[instrument]/ELECTRONANALYSER[electronanalyser]/transformations/@depends_on is required and hasn't been supplied by the reader.
The data entry corresponding to /ENTRY[entry]/SAMPLE[sample]/transformations/@vector is required and hasn't been supplied by the reader.
The data entry corresponding to /ENTRY[entry]/SAMPLE[sample]/transformations/@transformation_type is required and hasn't been supplied by the reader.
The data entry corresponding to /ENTRY[entry]/SAMPLE[samp
8000
le]/transformations/@depends_on is required and hasn't been supplied by the reader.

It seems like for several other fields now, the field name is removed while the attributes are directly added to the parent group name.

@rettigl
Copy link
Collaborator
rettigl commented Feb 22, 2024

Do we have/can we write unit tests that test for the correct behavior of inheritance in different scenarios?

@domna
Copy link
Collaborator Author
domna commented Feb 22, 2024

Yes, I thought that there is a misconception on my side as I just debugged this based on one key. But this makes sense from the change I introduced. Are you able to provide your files you use for conversion? It might be easier to test on the actual problem.

Do we have/can we write unit tests that test for the correct behavior of inheritance in different scenarios?

We have unit tests but obviously not for this particular case. Also coverage for it can be improved. I hope to add some additional tests with or after the verification is finished #133.

@domna
Copy link
Collaborator Author
domna commented Feb 22, 2024

I went through it again and I think the fix is fine. However, I forgot to change the path when writing the required keys to the template. I updated it and these errors are gone for me (at least for the NXmpes example) and the NxdlAttributeError is still gone.

@rettigl Could you give it a try once more?

@rettigl
Copy link
Collaborator
rettigl commented Feb 22, 2024

Works now for me as well

rettigl
rettigl approved these changes Feb 22, 2024
View reviewed changes
Copy link
Collaborator
@rettigl rettigl left a comment

Choose a reason for hiding this comment

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

While I don't understand the details, it fixes my issue, and does not seem to introduce other errors

@domna domna merged commit aef6ccd into master Feb 23, 2024
@domna domna deleted the fix-inherited-child-retrieval branch February 23, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherjeelshabih sherjeelshabih sherjeelshabih left review comments

@rettigl rettigl rettigl approved these changes

Assignees

@domna domna

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@domna @coveralls @rettigl @sherjeelshabih
0