Use multiformat reader #6
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
domna
force-pushed
the
use-multiformat-reader
branch
from
22fd0ac
to
1d9201d
Compare
2 tasks
|
Not sure I understand how to use this. If I run the example 3 of specsscan, it produces the following warnings: On main of pynxtools-mpes there are no warnings. |
|
These are the warnings for the sed example 3: |
|
There was a small error I introduced in the multi format reader (not sure why the ci didn't fail on this though). It should be fixed now. This is what I'm getting with specsanalyzer now: this seems to be a bug with the linking. I'll investigate and fix it. |
|
This is actually kind of confusing. In the config file it reads but in the file it writes If I change the config to everything is fine and without errors. Do you write the |
Yes, exactly, the idea is to create links dynamically depending on which axis is actually scanned. For this, I write the link string into the attrs. I added extra code to resolve links once more after template filling here: |
Yes. I forgot about this part, sorry. I'll add it to the multiformat reader |
|
I added it. I don't get any warnings with the specsanalyzer anymore. |
Thanks, but this still needs the fix you did in c1ecb70 I think. |
|
I added it now |
|
This works well for files where all entries are present. Created files are the same as per h5diff (apart from @file_time and @file_update_time). Which the old reader did not. The reason is that this entry is None in the dict. But I can change this in sed, and fill 0. However, I think the "!" notation does not work properly/the same way as in the old reader. Both in sed and in specsscan, the corresponding tests fail: From sed: Old reader: |
Yes, this is possible. We can just pass multiple objects to I actually use multiple entry writing for the igor reader. Can be implemented very similar here. |
Yes, I have seen this, but I did not quite understand how this is reflected in the config. I will have a closer look. |
I prefix the internal igor dict with the current entry name, that way I can automatically get the path from the current entry. We could also prefix the xarray's by entry name then we wouldn't need to change anything in the config file (we could just enumerate |
|
@rettigl Which test is this? I tried to get this logging output in specsanalyzer but I'm not able to find which test produces this output |
|
lukaspie
reviewed
Jul 26, 2024
Co-authored-by: Lukas Pielsticker <[email protected]>
lukaspie
approved these changes
Jul 26, 2024
lukaspie
reviewed
Jul 26, 2024
That wouldn't have worked here, because I did it via the gh web interface. But I can add it |
rettigl
requested changes
Jul 26, 2024
Comment on lines
+222
to
+226
| if ( | ||
| isinstance(objects, tuple) | ||
| and len(objects) > 0 | ||
| and isinstance(objects[0], xr.DataArray) | ||
| ): |
There was a problem hiding this comment.
One could also handle dicts here as ELN entries.
pynxtools_mpes/reader.py
Outdated
| try: | ||
| return iterate_dictionary(self.data_xarray.attrs, path) | ||
| except KeyError: | ||
| logger.info(f"Path {path} not found. Skipping the entry.") |
There was a problem hiding this comment.
If the key is not found in attrs, but maybe later in eln, this info is still issued, no? Isn't there a corresponding warning also in the multi reader, that collects all these?
* remove mappings, and adopt eln retrieval to use path rather than key * enable tests temporarily * Add units to h5 test data * re-disable tests * Add tests for new eln data formalism * Cleanup eln testing file --------- Co-authored-by: domna <[email protected]>
|
Done. I went with v0.2 since there were some major changes how the reader works internally. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the reader to use the multi format reader to align reading of json and other files.
This is the original PR for pynxtools: FAIRmat-NFDI/pynxtools#250