New types of datasets supported for Delmic HDF5 format

[noemiebonnet]

New supported formats: intensity, hyperspectral, angle-resolved, E-k, time-resolved.

Progress of the PR

• Read intensity,
• Read hyperspectral,
• Read angle-resolved,
• Read E-k,
• Read time-resolved,
• Read original_metadata,
• Read EELS,
• Write EELS,
• update docstring (if appropriate),
• update user guide (if appropriate),
• add a changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
• Check formatting of the changelog entry (and eventual user guide changes) in the docs/readthedocs.org:rosettasciio build of this PR (link in github checks)
• add tests,
• increase test coverage for errors,
• ready for review.

[codecov]

Codecov Report

❌ Patch coverage is 91.42857% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.08%. Comparing base (6ea01bd) to head (e6e1985).
⚠️ Report is 73 commits behind head on main.

Files with missing lines	Patch %	Lines
rsciio/delmic/_api.py	91.42%	15 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
+ Coverage   88.02%   88.08%   +0.06%     
==========================================
  Files          91       91              
  Lines       11538    11798     +260     
  Branches     2131     2186      +55     
==========================================
+ Hits        10156    10392     +236     
- Misses        875      890      +15     
- Partials      507      516       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[noemiebonnet]

pre-commit.ci autofix

[jlaehne]

Thanks @noemiebonnet for the substantial progress! I left a few comments from a first browsing over the code, but did not yet have time for a closer look. Please include the standard tracking list in the initial comment to get an overview of how far the PR is.

There are still a number of lines uncovered by tests, as commented by codecov. If lines should explicitly be ignored in the coverage test, one can add the comment # pragma: no cover at the end of the line. I see that most uncovered lines concern warnings or errors that might surface during file reading. It might be hard to test for those if one has only proper test files. What is our current best-practice in that case @ericpre? (Note that you can also get inspiration on such cases from other files readers recently implemented, such as Horiba JobinYvon or Hamamatsu).

[jlaehne]

A single spectrum is still loaded as <Signal1D, title: , dimensions: (1, 1|2048)>, but should be <Signal1D, title: , dimensions: (|2048)> (drop the navigation dimensions).

[jlaehne]

Currently, all the various loading errors are giving codecov warnings, because they are not included in the tests. In hdf5, it should be rather straight forward to create some minimal test files for each case where certain elements are deleted from the file to test each of these cases. An idea could also be to use hdf5 files from some other rsciio plugins that do not match the delmic specifications to test some of the errors.

[jlaehne]

As an example for testing errors, here you could use the file from a different type of data and change the img_type to get a case that triggers this error.

[jlaehne]

Some other readers have a parameter to choose whether to read such axes as a UniformDataAxis by calculating the mean scale, or as a non-uniform DataAxis by just taking exactly the vector that is in the file.

see e.g.:

rosettasciio/rsciio/jobinyvon/_api.py

Line 684 in 1274ed5

use_uniform_signal_axis : bool, default=False

[jlaehne]

@noemiebonnet is it possible to update the progress you have made on your local branch? Might be enough to include the current PR in the release planned for the next days: #381

[aidanc151]

I'm running into an error message after converting a .h5 into a .hspy, and then trying to load the .hspy:

TypeError: issubclass() arg 1 must be a class

To reproduce:

1. Load a .h5 hyperspectral map
2. Use the .save wrapper and save into .hspy format
3. Load the new .hspy file

It looks like it might be related to the original metadata - as if the initial loading of .h5 is done without the original metadata, the error is avoided, e.g.:

hs.load('hyperspectral.h5', reader = 'delmic', load_original_metadata=False)

[jlaehne]

It looks like it might be related to the original metadata

Indeed, the parsing of original_metadata is still wrong. I inspected the resulting dictionary, e.g.:

from rsciio.delmic import file_reader
d1 = file_reader('delmic_file.h5')
d1.[0]['original_metadata']

and it results in:

{'BackprojectedIlluminationPinholeRadius': array([2.8e-07]),
...

All values are read in as array, no matter of their content - none of them actually seem to contain more than one array element. In particular, nested dictionaries such as for the 'ExtraSettings' are not correctly parsed by HyperSpy if the dictionary is embedded in an array.

[pieleric]

I will pick up this PR from now on. I went through all the code and the comments of this pull-request and attempted to address all the issues raised. Here is a summary of the (major) update:

• Major overhaul of the code structure, to be more generic, and handle all (corner) cases.
• Adjust the behaviour of "signal" argument: it now defaults to passing all the data, and all the possible options are lower-case only.
• The metadata now also include acquisition time and dataset title.
• The original_metadata now also include the SVIData and acquisition time.
• Ensure that the original_metadata dict only contains basic Python types (ie, no numpy array or JSON-encoded strings)
• Fix offset value in the X & Y which were incorrectly computed.
• Fix incorrect direction of the X axes in 4D angular-resolved datasets.
• If LumiSpy is installed, use the corresponding Signal type for dataset matching CL_SEM, LumiTransient, and LumiTransientSpectrum types.
• Support angular-resolved data with multiple polarizations.
• Remove some test cases which were only testing the same as another one.
• Add test case attempting to load incorrect HDF5 file.

I now see that some CI tests are failing (though all the test cases did pass on my computer). I'll dig into these issues tomorrow.

Any feedback is already well appreciated!

[jlaehne]

We added basic support in the 0.7 release:
https://rosettasciio.readthedocs.io/en/latest/changes.html#id42

So the changelog should make clear that more complete support for different types of acquistions is now added.

[jlaehne]

We were discussing what is actually the best default here - reading only CL to return a single signal item as in most other readers or the list with all items, but I guess the main point would be that it is well documented.

Also, I am not certain having the survey first is the best order of the signals in the list. I assume the order is motivated by the odemis files? Generally, I would find it more intuitive to have the actual CL signal first, then concurrent and last survey (making the survey always last item, even if there are multiple streams in one file).

Finally, I would say there is not really a point in reading in the concurrent se when it is dimensionless (for spectra, single transients or streak images).

[jlaehne]

In particular having the anchor region data from drift correction included in the default loading seems a bit overload - so maybe default to cl and have an option all to include the other datasets?

[pieleric]

Sure. I've now changed it to return only the "cl" datasets by default. Also changed from None to "all" to get all the datasets.

Concerning the datasets with a single point, although it's probably not useful, I'd rather always return them when that type of dataset (or "all") is requested. This keeps the code simple and makes sure that if for some reason the user actually do care about this single pixel, they can still read it.

[jlaehne]

Fair enough for the datasets with a single point. Only, I would still tend to reverse the order of the different signals in the list for "all" to have the 'cl' first (as it is the main one and now also the one that is read by default) and then add the additional signals with 'survey' last.

[jlaehne]

Thanks Eric for picking up on this. In general it looks good, and some first tests with real data did not surface any problems.

We are actually interested in getting this released ASAP. So I would try to resolve anything that needs to be decided in terms of API (see some comments on the signal parameter) and as soon as tests run through merge the PR.

In the mid-term, additional features such as metadata parsing and features such as reading in scan areas/points on the survey as markers could be added via separate PRs. Coverage is not optimal, but missing lines mainly seem to concern errors at could maybe also be addressed later.

[pieleric]

Thanks Eric for picking up on this. In general it looks good, and some first tests with real data did not surface any problems.

We are actually interested in getting this released ASAP. So I would try to resolve anything that needs to be decided in terms of API (see some comments on the signal parameter) and as soon as tests run through merge the PR.

In the mid-term, additional features such as metadata parsing and features such as reading in scan areas/points on the survey as markers could be added via separate PRs. Coverage is not optimal, but missing lines mainly seem to concern errors at could maybe also be addressed later.

Thanks Jonas for the review. I've tried to correct the code for all your comments. I agree with the way-forward. Let's try to get this big change merged in, and then we can more easily bring extra features later on in "small bites".

Let me know if you want me to squash some of the commits together before merging the PR.

[pieleric]

I've tried hard to get the packaging test to pass, but I'm now at loss at how to handle the current errors. I assume it's related to the way the test data is stored... but no idea what should be changed to fix it, or even if that's a real error. Do you have any hint on how to solve such error?

ERROR tests/test_import.py::test_import_version - requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://github.com/noemiebonnet/rosettasciio/raw/extend-delmic-format/rsciio/tests/data/zspy/signal1d_10x10-DirectoryStore.zspy/.zattrs

[ericpre]

I don't remember the exact reason, but this is related to a recently merged PR #417 that add these test files and these are not included in this branch. A rebase should fix it.

[jlaehne]

Let me know if you want me to squash some of the commits together before merging the PR.

Maybe squash the last few commits with only minor corrections into one.

[pieleric]

I've updated the pull-request with these changes:

• sort the returned data to be in the order CL, SE, survey, anchor
• fix the documentation according to the comments by Jonas
• rebased on the latest main (to avoid the test failures hopefully)
• Simplified the test cases, so that each call to the loader is done only once
• improved coverage by testing a few more cases (data with anchor region, incorrect arguments) and adding some pragma's around the most unlikely exception handlers.

Talking of coverage, I don't have the tools to measure it locally, so it's a bit hard to check how well it will go. Also it seems the coverage is considered a "fail" if it's below the project average, which is currently at 87%. Please be aware that mathematically, if every merge is above the average, this average will increase, which will make the bar to entry higher and higher for the next pull-requests, which will make the work of submitters harder and harder...

[jlaehne]

Thanks Éric :-)

* sort the returned data to be in the order CL, SE, survey, anchor

My thought was to have survey last so that it can always be accessed as the [-1] element of the list (and anchor is not always there).

[jlaehne]

Talking of coverage, I don't have the tools to measure it locally, so it's a bit hard to check how well it will go. Also it seems the coverage is considered a "fail" if it's below the project average, which is currently at 87%. Please be aware that mathematically, if every merge is above the average, this average will increase, which will make the bar to entry higher and higher for the next pull-requests, which will make the work of submitters harder and harder...

You can see the current report and a markup of missing lines at Codecov, but you have reached the threshold now:
https://app.codecov.io/gh/hyperspy/rosettasciio/pull/328?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=hyperspy

The aim should be close to 100% coverage for the relevant parts of new contributions (and most of the rest can usually be ignored by pragmas). It is well below 100% mainly because some of the oldest readers started out without any tests and were never brought close to this value, but indeed we have improved over the years. In the end, the check is mainly to have a guideline where we are heading and it is at the discretion of the maintainers to merge PRs even with a below average coverage. From experience, it is good to directly push for good coverage instead of postponing that to separate PRs.

[ericpre]

There will be an expected failure on the build (the one labelled "hyperspy_dev") with the development branch of hyperspy. This can be ignored in this PR.

For the coverage, if it doesn't pass the currently set criteria, it is not a big deal, usually what we do is to check online (using the link in the "github checks") where the code is missed and correct where needed.

[jlaehne]

rebased on the latest main (to avoid the test failures hopefully)

Seems to have solved all but one hspy-related test failure. @ericpre ?

[ericpre]

Yes, these are the one that are expected and sorted in #425 and hyperspy/hyperspy#3528.

[pieleric]

My thought was to have survey last so that it can always be accessed as the [-1] element of the list (and anchor is not always there).

Ok. Changed the order to CL, SE, anchor, survey.

[jlaehne]

Thanks @pieleric - that is a great step forward.

TODO:

• Mapping from original_metadata to metadata according to the LumiSpy+HyperSpy(SEM) conventions
• Reading of scan area / spectral position on survey image as marker