Restandardize handling of Multi-Image files #2815

maaleske · 2017-09-28T13:17:21Z

Description

Fixes Inconsistent behaviour of ImageCollection #2811 for the described use case.
Adds simple unit tests for pattern expansion with PIL-readable and non-PIL-readable files.
Also fixes previously nonsensical string_sort test

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

Clean style in the spirit of PEP8
Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Unit tests

References

Closes #2811

For reviewers

(Don't remove the checklist below.)

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.

stefanv · 2017-10-03T22:56:51Z

The tests are legitimately failing on Travis-CI for this PR.

maaleske · 2017-10-10T13:05:13Z

I think I resolved the test failures by explicitly passing load_func=None in MultiImage to trigger the old behaviour. I could not find a reason for the special imread definition there, and removing it doesn't seem to affect tests. This feels slightly odd and also makes MultiImage quite redundant (all it does is set the _filename attribute).

The whole thing feel slightly fragile, since even specifying just load_func=imread automatically removes the flattening of multi-frame files. It seems no tests catch this with imread_collection, which does exactly that. I don't what the better way would be. Should there perhaps be an explicit parameter for enabling the flattening behaviour?

stefanv · 2017-10-10T17:53:56Z

My intuition is that we should never flatten. Each file should map to one image (array).

We can build a special interface to load multi-images and unpack them, but as you suggest, that should be explicit.

maaleske · 2017-10-13T11:12:27Z

I separated all the multi-image functionality from ImageCollection to a inherited class FrameCollection and implemented MultiImage on top of that. Now ImageCollection is completely frame-agnostic, and just sees each frame as its own image, with _files corresponding to the _frame_index. MultiImage is now just a simple passthrough to Framecollection, and I think it could probably be deprecated as it adds no real functionality (as was apparently discussed before: #1200). Any comments on the class organization?

There's still some niggles to fix before it's ready for merging:

The current _files should probably be renamed to _ids for clarity, since it doesn't consistently refer to files (possibly adding a property which always does?)
_frame_index needs to be updated when slicing FrameCollections (was in ImageCollection)
FrameCollection does not yet accept any special load_func

maaleske · 2017-10-16T13:51:12Z

I added the load_func parameter for FrameCollection, and resolved the slicing behaviour of _frame_index by removing it altogether (since it was redundant with _files in the returned collection). The original list of searched files can now be accessed as _searched_files, which should be helpful for anyone trying to debug further issues with multi-images.

Existing functionality should now work as it was, just with FrameCollection instead of ImageCollection.

stefanv · 2017-10-16T17:53:49Z

Doesn't FrameCollection and MultiImage now do the same when called on a multi-frame image?

maaleske · 2017-10-16T20:54:27Z

@stefanv Yes, FrameCollection is just the existing MultiImage, that now also handles multiple images. MultiImage probably should be deprecated in favor of FrameCollection, with warnings before removing it completely?

stefanv · 2017-10-16T21:54:11Z

Why not just replace MultiImage with the FrameCollection implementation? Do they have differing APIs?

maaleske · 2017-10-16T22:06:27Z

The API is different in that FrameCollection handles multiple multi-images. I'm not sure what you mean by replacing, do you want to keep the MultiImage class name (even though it now handles multiple files as well)?

stefanv · 2017-10-16T22:07:54Z

I think, since we only added functionality, and since the old calls will still work, we can probably keep the MultiImage name, yes.

maaleske · 2017-10-16T22:12:54Z

I figured it would be more clear to make a new class that explicitly handles any sets of frames, but I guess keeping MultiImage as the name is not too confusing.

stefanv · 2017-10-16T22:15:59Z

Another option is to add an unpack_frames keyword to ImageCollection, and deprecate MultiImage.

maaleske · 2017-10-16T22:20:27Z

Adding the parameter would make for a lot of branching in ImageCollection. Having the FrameCollection/MultiImage just inherit ImageCollection makes the latter much cleaner.

maaleske · 2017-10-16T22:26:55Z

There is the small API change of MultiImage having the filename property which doesn't make sense in a multiple file setting, and would be best removed (which I guess was why I thought about making the separate class in the first place).

stefanv · 2017-10-16T22:32:42Z

OK, then let's keep MultiImage, and just set the filename parameter to the file pattern, for backward compatibility. We can make it a property, and warn that that property is being deprecated.

stefanv · 2017-10-16T22:32:58Z

Or, just keep it, and have it contain the file pattern :) Since this is new functionality, no-one should notice.

stefanv

This looks great, thanks!

I left some minor nitpicks, but overall good to go.

Is your name in the CONTRIBUTORS file, by the way? Please add it.

stefanv · 2017-10-16T22:50:27Z

skimage/io/collection.py

+    load_pattern : str or list
+        Pattern glob or filenames to load. The path can be absolute or
+        relative.  Multiple patterns should be separated by os.pathsep,
+        e.g. '/tmp/work/*.png:/tmp/other/*.jpg'.


Backticks for code, ``/tmp/work/*.png:/tmp/other/*.jpg``

Same for ``os.pathsep`` above

stefanv · 2017-10-16T22:51:01Z

skimage/io/collection.py

-    The current implementation makes use of ``tifffile`` for Tiff files and
-    PIL otherwise.
+    By default, MultiImage builds a list of frames in each image
+    and calls ``imread(filename, img_num=n)`` to read the n:th frame in


Some indication that it is being called for each n

maaleske · 2017-10-16T23:08:11Z

Done. To summarize the current version as compared to master:

ImageCollection now always deals with single items (files, indices, etc.) set by load_pattern (and possibly load_func) and never flattens images.
MultiImage implementation is now inherited from ImageCollection and just passes it the _frame_index (as built previously by _find_images) and a wraps imread or load_func to accept the pairs as input.
As a resulting new feature, MultiImage can now handle multiple files with a special load_func (which must then accept the img_num parameter)

stefanv · 2017-10-16T23:19:29Z

This looks great; thanks @maaleske!

stefanv · 2018-05-01T21:46:56Z

@scikit-image/core This PR went through review and has been approved; if another person could take a look, rebase and merge, please.

mkcor · 2020-11-18T16:43:36Z

I have deleted my comment starting with "Sorry for the gross series of 'Remove extra lines' commits..." -- I thought I wouldn't be able to push from my local branch and, hence, had to work with the online editor (because I couldn't when I wasn't core, but anyway).

mkcor · 2020-11-18T23:23:14Z

I have tried updating this PR in line with #3928 but I'm afraid I have lost the original spirit of this contribution along the way... 😞 There are now 3 failing tests in test_collection.py, which were added by the original author, i.e.:

FAILED io/tests/test_collection.py::TestImageCollection::test_custom_load - T...
as expected since load_pattern = [(1, 'one'), (2, 'two')] is not a string nor a list of strings;
FAILED io/tests/test_collection.py::TestImageCollection::test_custom_pattern
indeed, glob('PREFIX/.cache/scikit-image/master/data/b??ck.png') results in ['PREFIX/.cache/scikit-image/master/data/brick.png'] and not in ['PREFIX/.cache/scikit-image/master/data/block.png', 'PREFIX/.cache/scikit-image/master/data/brick.png'] as 'expected;'
FAILED io/tests/test_collection.py::TestImageCollection::test_custom_pattern_not_pil
where pattern PREFIX/.cache/scikit-image/master/data/chessboard_*_U8.npz' doesn't get matched to the expected ['chessboard_GRAY_U8.npz', 'chessboard_RGB_U8.npz']

Note that function _find_files doesn't get used at this point.

@jni could you please have a look? Thanks 😬

mkcor · 2020-11-19T00:46:12Z

Forgot to loop in @alexdesiqueira, especially when referring to #3928 🙏

mkcor · 2020-11-19T16:43:29Z

I realize this PR is three years old, so it might be unrealistic to include it at this point... I thought I would give it a try, since it was tagged with action: mrg+1 as well as 0.18.

Note that function _find_files doesn't get used at this point.

Removed in fad090c.

* FAILED io/tests/test_collection.py::TestImageCollection::test_custom_load - T...
  as expected since `load_pattern = [(1, 'one'), (2, 'two')]` is not a string nor a list of strings;

Removed in 335be36.

maaleske · 2020-11-19T17:02:26Z

Since the pattern parameter was changed, the tests I added are probably outdated since they were meant to check for differences w.r.t. the original behaviour. It's probably best to disregard them and just make sure that you test whatever is needed to check for the issues in #5069 and #2811 (if that one still happens).

mkcor · 2020-11-19T20:04:13Z

@maaleske great to read you!! Sure. Specifically, we want test_custom_load to fail, because the current approach is to support only str and list of str patterns. That's why I removed it. But, in my understanding so far, I think we would want test_custom_pattern to pass... Good point anyway, about focusing on the issues which are currently open. Thank you!

alexdesiqueira · 2020-11-20T19:11:16Z

@maaleske great to see you around, brother! Hope we can get a beer one of these days.
@mkcor I'm sorry, I didn't see we're working on this at #4859 — I told* @jni I would check but it slipped away. Could we check that one instead?

mkcor · 2020-11-20T20:45:48Z

Oh, I wasn't aware of #4859... 🤦‍♀️ It's okay: I've spent enough time in this code by now that I would be fast at reworking either PR. Which one of the two PRs do we want to close, @jni? @alexdesiqueira I don't think we want to keep both in parallel, indeed.

My general comment remains the same: My understanding is that we want to remain in the spirit of #3928 and build from there. Notably, we want to accept loading patterns which are either strings or lists of strings (not lists of anything as in the original contribution). Besides, if we keep (as I suggest we do) the handling of multi-pattern brought by #3928, the detection of frames as implemented in the original contribution fails at splitting frames within a given (multi-image) file. Therefore, we can only keep a marginal part of this contribution on the functionality side. But we should keep most tests brought by this contribution (yay!) even if some need adapting.

jni · 2020-11-21T01:00:14Z

@mkcor @alexdesiqueira I gotta say I don't understand this stuff anymore! 😅 To me it seems that either PR can go forward, #4859 was a simple rebase. Since @mkcor you have made some nice updates here, I think we can just close #4859 and keep going here!

alexdesiqueira · 2020-11-21T01:03:36Z

@mkcor @jni I apologize, that was on me. Thank you for understanding Juan!

mkcor · 2020-11-27T19:38:33Z

@jni I don't know if we can squeeze this fix into 0.18... Anyway, I'm happy to be done with this resurrected PR! 😌

@cosinus11 eeffe8a fixes #5069.

In the end, I respected the original vision outlined by @maaleske and @stefanv:

My intuition is that we should never flatten. Each file should map to one image (array).
We can build a special interface to load multi-images and unpack them, but as you suggest, that should be explicit.

Namely, an image collection of (multiple) multi-frame file(s) has a tree structure when specifying a loading function (load_func parameter): ic[0][0] is the first frame from the first file, ic[0][1] is the second frame from the first file, etc.

The changes brought by this PR don't overwrite those brought by #3928 -- still, it would be great if you could review, @alexdesiqueira! 😄

mkcor · 2020-11-27T19:43:10Z

Additional notes:

d3a968e draws from ImageCollection is able to receive lists of patterns #3928 (comment);
03d2936 is pretty much the same as the example I updated in the docstring at eeffe8a;
for the above-mentioned example, I have drawn inspiration from https://scikit-image.org/docs/dev/user_guide/video.html#imageio.

cosinus11 · 2020-12-08T17:07:31Z

@jni I don't know if we can squeeze this fix into 0.18... Anyway, I'm happy to be done with this resurrected PR! 😌

@cosinus11 eeffe8a fixes #5069.

In the end, I respected the original vision outlined by @maaleske and @stefanv:

My intuition is that we should never flatten. Each file should map to one image (array).
We can build a special interface to load multi-images and unpack them, but as you suggest, that should be explicit.

Namely, an image collection of (multiple) multi-frame file(s) has a tree structure when specifying a loading function (load_func parameter): ic[0][0] is the first frame from the first file, ic[0][1] is the second frame from the first file, etc.

The changes brought by this PR don't overwrite those brought by #3928 -- still, it would be great if you could review, @alexdesiqueira! 😄

Thank you very much for your attention and your reactivity, amazing!
For your information, I use ImageCollection for reading 3D images for tomography (one file for 2048x2048x2048 voxels), in connection with PIL I read this images directly in raw format provided by the reconstruction software.

stefanv · 2020-12-12T03:00:25Z

Thank you all for your feedback and work put into this PR; given that this has been reviewed by more eyes than most other PRs, I am putting it in. Thanks @maaleske !

jni · 2020-12-12T07:58:20Z

@meeseeksdev backport to v0.18.x

…files

jni · 2020-12-12T07:59:45Z

Wow, thank you so much @maaleske for the original work, and @alexdesiqueira @mkcor for the resurrection, and @stefanv for the merge! Amazing to not have this looming over my head anymore! 😂

Co-authored-by: Matti Eskelinen <matti.a.eskelinen@gmail.com>

alexdesiqueira · 2020-12-12T15:32:48Z

Thank you very much everyone! Sorry for not reviewing again* @mkcor, thank you for your work!

soupault added 💪 Work in progress ⏩ type: Enhancement Improve existing features labels Oct 3, 2017

maaleske force-pushed the fix-numframes branch from 5899e76 to b072f05 Compare October 10, 2017 11:19

maaleske changed the title ~~[WIP] Fix inconsistency of _numframes in ImageCollection~~ [WIP] Restandardize handling of Multi-Image files Oct 13, 2017

stefanv approved these changes Oct 16, 2017

View reviewed changes

stefanv added status: mrg+1 and removed 💪 Work in progress labels Oct 30, 2017

soupault added this to the 0.14 milestone May 6, 2018

soupault changed the title ~~[WIP] Restandardize handling of Multi-Image files~~ Restandardize handling of Multi-Image files May 6, 2018

maaleske added 2 commits May 22, 2018 23:15

Fix nonsensical string_sort test

15bb4f0

Fix whitespace issues

ddaec3e

mkcor added 3 commits November 18, 2020 19:15

Clean up multi-img tests

460376b

Clean up image collection init

50894bf

Reinclude frame search

1af7806

mkcor added 2 commits November 19, 2020 17:30

Remove unused function

fad090c

Remove test where load pattern is a list of not str

335be36

mkcor added 4 commits November 27, 2020 16:27

Keep new syntax only

d3a968e

Update test for custom load_func

03d2936

Remove flattening in multi-img tests

e5df785

Remove flattening of multi-frame files

eeffe8a

stefanv merged commit e0ad3f6 into scikit-image:master Dec 12, 2020

meeseeksmachine mentioned this pull request Dec 12, 2020

Backport PR #2815 on branch v0.18.x (Restandardize handling of Multi-Image files) #5132

Merged

meeseeksmachine pushed a commit to meeseeksmachine/scikit-image that referenced this pull request Dec 12, 2020

Backport PR scikit-image#2815: Restandardize handling of Multi-Image …

c134a22

…files

jni pushed a commit that referenced this pull request Dec 12, 2020

Backport PR #2815: Restandardize handling of Multi-Image files (#5132)

e4ce422

Co-authored-by: Matti Eskelinen <matti.a.eskelinen@gmail.com>

Uh oh!

Restandardize handling of Multi-Image files #2815

Restandardize handling of Multi-Image files #2815

Uh oh!

Conversation

Uh oh!

Description

Checklist

References

For reviewers

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!