Add support for reading `mib` acquired with a given number of frame to skip per line

emichr commented 1 month ago

Description of the change

A few sentences and/or a bulleted list to describe and motivate the change:

Added a function to read the navigation shape and check for lineskips of a .mib dataset from the corresponding .hdr file. This should help with #270.
Fixed some typos in various docstrings.

Progress of the PR

[x] Change implemented (can be split into several points),
[x] update docstring (if appropriate),
[ ] update user guide (if appropriate),
[x] add a changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
[x] 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,
[x] ready for review.

Minimal example of the bug fix or the new feature

from rsciio.quantumdetector._api import parse_hdr_file, _get_navigation_shape_from_hdr
hdr = parse_hdr_file("SPEDSTEM_256x256_8cm_100pct.hdr")
# Your new feature...
navigation_shape = _get_navigation_shape_from_hdr(hdr)
print(navigation_shape)

SPEDSTEM_256x256_8cm_100pct.txt

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 29.16667% with 17 lines in your changes missing coverage. Please review.

Project coverage is 86.20%. Comparing base (bcb2057) to head (a4d40ba). Report is 3 commits behind head on main.

Files	Patch %	Lines
rsciio/quantumdetector/_api.py	29.16%	14 Missing and 3 partials :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #272 +/- ## ========================================== - Coverage 86.33% 86.20% -0.13% ========================================== Files 83 83 Lines 10668 10692 +24 Branches 2330 2337 +7 ========================================== + Hits 9210 9217 +7 - Misses 939 953 +14 - Partials 519 522 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ericpre commented 1 month ago

Thanks @emichr. As mentioned in https://github.com/hyperspy/rosettasciio/issues/270#issuecomment-2158637124, the logic needs to take into account interrupted acquisition.

Can you make and add small test files to cover this use case (and different number of frame to skip per line)? The test suite fails with this PR:

FAILED rsciio/tests/test_quantumdetector.py::test_interrupted_acquisition - assert (9,) == (4, 2)
FAILED rsciio/tests/test_quantumdetector.py::test_non_square[navigation_shape1] - assert (4, 2) == (8,)
FAILED rsciio/tests/test_quantumdetector.py::test_first_last_frame_8[kwargs0-navigation_shape1] - assert (8,) == (4, 2)
FAILED rsciio/tests/test_quantumdetector.py::test_first_last_frame_8[kwargs1-navigation_shape1] - assert (8,) == (4, 2)
FAILED rsciio/tests/test_quantumdetector.py::test_first_last_frame_8[kwargs2-navigation_shape1] - assert (8,) == (4, 2)
FAILED rsciio/tests/test_quantumdetector.py::test_first_last_frame_8[kwargs3-navigation_shape1] - assert (8,) == (4, 2)
FAILED rsciio/tests/test_quantumdetector.py::test_first_last_frame_8[kwargs4-navigation_shape1] - assert (8,) == (4, 2)
FAILED rsciio/tests/test_quantumdetector.py::test_first_last_frame_8[kwargs5-navigation_shape1] - assert (8,) == (4, 2)
FAILED rsciio/tests/test_quantumdetector.py::test_first_last_frame_8[kwargs6-navigation_shape1] - assert (8,) == (4, 2)
FAILED rsciio/tests/test_quantumdetector.py::test_navigation_shape_list_error - Failed: DID NOT RAISE <class 'TypeError'>

emichr commented 1 month ago

Thanks @emichr. As mentioned in #270 (comment), the logic needs to take into account interrupted acquisition.

Can you make and add small test files to cover this use case (and different number of frame to skip per line)? The test suite fails with this PR:

Sure, I'll look into it as soon as possible!

emichr commented 1 month ago

@ericpre There was a bug in my PR that caused the tests to fail. I fixed the bug and it should run fine now - sorry for not checking sooner!

I plan on making some test data soon with the updated Merlin software with various linetriggers, but it will take some time.

ericpre commented 1 month ago

This sounds good. Are you okay with including the test file as part of the PR? I would like to review with the test file. When you make the test file, can you also make one with interrupted acquisition?

Would it make sense to actually skip the frame(s) (by slicing the data array in the right place?), instead of increasing the navigation shape?

emichr commented 1 month ago

This sounds good. Are you okay with including the test file as part of the PR? I would like to review with the test file. When you make the test file, can you also make one with interrupted acquisition?

I probably won't be able to make the test data files this week, so the tests will have to wait a bit. I'm okay with waiting for those and including them in the PR when ready.

Would it make sense to actually skip the frame(s) (by slicing the data array in the right place?), instead of increasing the navigation shape?

Yes, that's a good idea. It required very little changes, and I think I found the right place to do it. Let me know if it's not the best way of doing it.

hyperspy / rosettasciio