ImageCollection is able to receive lists of patterns #3928

alexdesiqueira · 2019-05-24T19:40:31Z

Description

Users can pass a list of patterns to ImageCollection, instead of a string.

Checklist

Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests
Clean style in the spirit of PEP8

For reviewers

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.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

alexdesiqueira · 2019-05-24T20:09:27Z

OK, I'll consider that the implemented tests think on load_pattern as being images themselves, already.

jni · 2019-05-25T01:08:46Z

skimage/io/collection.py

@@ -104,6 +98,9 @@ class ImageCollection(object):

    Notes
    -----
+    Note that files are always stored in alphabetical order. Also note that


This is false, it's alphanumeric order, im1.jpg will come before im10.jpg. My second contribution ever to scikit-image. =D

jni · 2019-05-25T01:10:50Z

skimage/io/collection.py

    """

    def __init__(self, load_pattern, conserve_memory=True, load_func=None,
                 **load_func_kwargs):
        """Load and manage a collection of images."""
-        if isinstance(load_pattern, str):
+        try:
+            if not isinstance(load_pattern, str):


I think this check should be more stringent: it should not be a string, but it should be a sequence.

Also, try/except clauses should contain as little content as possible. I think this one encompasses too many statements.

Tried to fix that with:

if isinstance(load_pattern, (list, tuple)):

IMHO this try/except is as good as it gets. It receives the three types of inputs we'd like to put there, and it's rather efficient when compared to an if isinstance(load_pattern, (str, list, tuple). That could also bring us issues in a more difficult type of input:

load_pattern = [(1, 'one'), (2, 'two')] In []: isinstance(load_pattern, (list, tuple)) Out[]: True

I don't know... Is there something I'm missing here? How could we solve that other way? Thanks for your help Juan!

jni · 2019-05-25T01:12:33Z

skimage/io/collection.py

-        relative.  Multiple patterns should be separated by os.pathsep,
-        e.g. '/tmp/work/*.png:/tmp/other/*.jpg'.  Also see
-        implementation notes below.
+        relative. Also see implementation notes below.


I think we should include all possible inputs here, including the pathsep. As far as I know we don't intend to deprecate that? We could keep the original message but include the statement about windows vs *nix, and also the option to pass in a list of patterns.

They still work; I tried to state that in the documentation. Is it good?

jni

A great start! And thanks for tackling this! In addition to my minor comments, you should include a test for the new behaviour.

jni · 2019-05-26T01:39:12Z

skimage/io/collection.py

+
+    If multiple patterns are entered as a single string, they should be
+    separated by os.pathsep, e.g. '/tmp/work/*.png:/tmp/other/*.jpg' in *nix,
+    or '/tmp/work/*.png;/tmp/other/*.jpg' in Windows.


jni · 2019-05-26T02:07:56Z

@alexandrejaguar the problem with the TypeError syntax is that there's lots of statements there, any of which could potentially raise a TypeError that is not actually the error we're looking for.

There is also a design philosophy that I mostly subscribe to that "exceptions are for exceptional circumstances", and you shouldn't use them for "normal" and "expected" logic flow.

My suggestion: create a helper function, is_multipattern, that checks the following condition:

import typing

((isinstance(load_pattern, str) and os.pathsep in load_pattern) or
 (not isinstance(load_pattern, str) and
  isinstance(load_pattern, typing.Sequence) and
  all(isinstance(pattern, str) for pattern in load_pattern))
)

Then:

if is_multipattern(load_pattern):
    if isinstance(load_pattern, str):
        load_pattern = load_pattern.split(os.pathsep)
    ...
elif isinstance(load_pattern, str):
    ...  # single pattern logic
else:
    # Raise TypeError

This might also be a good opportunity for us to start using progressive typing:

from typing import Sequence, Union as U

def imread_collection(load_pattern : U[str, Sequence[str]], ...):

Then people's IDEs, at least, will tell them that [(1, 'one')] is invalid input for the function.

alexdesiqueira · 2019-05-26T16:42:17Z

@jni sounds good, let's see what I got.

stefanv · 2019-05-27T06:46:16Z

On Fri, 24 May 2019 18:12:33 -0700, Juan Nunez-Iglesias wrote: > Parameters ---------- 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'. Also see - implementation notes below. + relative. Also see implementation notes below. I think we should include all possible inputs here, including the pathsep. As far as I know we don't intend to deprecate that? We could keep the original message but include the statement about windows vs *nix, and also the option to pass in a list of patterns.

This is a tricky point: on the one hand you want the docstring to give an accurate description of the function, but on the other you want it to encourage best behavior. It may be best to be honest: "For backward compatibility, we also accept a string of patterns delimited by `os.pathsep`."

jni · 2019-05-27T09:18:52Z

It may be best to be honest: "For backward compatibility, we also accept a string of patterns delimited by os.pathsep."

I agree with this wording, thanks @stefanv! I would add only "... delimited by os.pathsep (which is ":" on Mac and Linux platforms, and ";" on Windows)."

I think it's important to be clear on this in case someone is trying to figure out what's wrong with the code that was written on one platform and is now broken on another.

alexdesiqueira · 2019-05-27T11:17:44Z

It may be best to be honest: "For backward compatibility, we also accept a string of patterns delimited by os.pathsep."

I agree with this wording, thanks @stefanv! I would add only "... delimited by os.pathsep (which is ":" on Mac and Linux platforms, and ";" on Windows)."

I think it's important to be clear on this in case someone is trying to figure out what's wrong with the code that was written on one platform and is now broken on another.

I'm more into the wordy version as well, with a little modification: "... delimited by os.pathsep (";" on Windows, ":" on Mac and Linux)."

jni · 2019-05-27T11:35:16Z

@alexandrejaguar nice job making it more concise! I'm all for it!

alexdesiqueira · 2019-05-27T12:20:13Z

Then people's IDEs, at least, will tell them that [(1, 'one')] is invalid input for the function.

@jni I understand the concern with TypeError, and thanks for your example! Let's be clear then: we'll not accept entries as [(1, 'one'), (2, 'two')] in ImageCollection() anymore?
If not, we'll have to remove one of the tests as well.

pep8speaks · 2019-05-27T12:26:33Z

Hello @alexdesiqueira! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file skimage/io/collection.py:

Line 91:42: W504 line break after binary operator

Comment last updated at 2019-09-17 00:54:07 UTC

jni · 2019-05-27T12:30:40Z

Let's be clear then: we'll not accept entries as [(1, 'one'), (2, 'two')] in ImageCollection() anymore?

Wait, what? We accept those? What does it mean?

alexdesiqueira · 2019-05-27T12:35:02Z

There's a test that check for input like that, maybe a list of previously input images... I was trying to deal with that actually =D Yep, maybe it's time to stop considering that...

stefanv · 2019-05-27T17:28:34Z

If it is supported API with a test I would be very hesitant to remove it without further investigation. Was this perhaps to support "frame generators" that output frame number + image file combos? Or perhaps a way to provide explicit ordering? Is it used internally that way, without ever having been externally advertised? In that case, we could potentially just change it.

alexdesiqueira · 2019-05-27T21:51:09Z

If it is supported API with a test I would be very hesitant to remove it without further investigation.

@stefanv @jni that's what I'm afraid of. This is the test:

================================== FAILURES ===================================
____________________ TestImageCollection.test_custom_load _____________________
self = <skimage.io.tests.test_collection.TestImageCollection testMethod=test_custom_load>
    def test_custom_load(self):
        load_pattern = [(1, 'one'), (2, 'two')]
    
        def load_fn(x):
            return x
    
>       ic = ImageCollection(load_pattern, load_func=load_fn)
python35-x64\lib\site-packages\skimage\io\tests\test_collection.py:83: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <skimage.io.collection.ImageCollection object at 0x000000B388F8FF60>
load_pattern = [(1, 'one'), (2, 'two')], conserve_memory = True
load_func = <function TestImageCollection.test_custom_load.<locals>.load_fn at 0x000000B3885900D0>
load_func_kwargs = {}
    def __init__(self, load_pattern, conserve_memory=True, load_func=None,
                 **load_func_kwargs):
        """Load and manage a collection of images."""
        self._files = []
        if _is_multipattern(load_pattern):
            if isinstance(load_pattern, str):
                load_pattern = load_pattern.split(os.pathsep)
            for pattern in load_pattern:
                self._files.extend(glob(pattern))
            self._files = sorted(self._files, key=alphanumeric_key)
            self._numframes = self._find_images()
        elif isinstance(load_pattern, str):
            self._files.extend(glob(load_pattern))
            self._files = sorted(self._files, key=alphanumeric_key)
            self._numframes = self._find_images()
        else:
>           raise TypeError('Invalid pattern as input.')
E           TypeError: Invalid pattern as input.
python35-x64\lib\site-packages\skimage\io\collection.py:184: TypeError

Is it used internally that way, without ever having been externally advertised?

I'll ckeck it. 🔍

alexdesiqueira · 2019-05-27T21:59:47Z

Got it (skimage/io/tests/test_fits.py):

@testing.skipif(not pyfits_available, reason="pyfits not installed")
def test_imread_collection_single_MEF():
    io.use_plugin('fits')
    testfile = os.path.join(data_dir, 'multi.fits')
    ic1 = io.imread_collection(testfile)
    ic2 = io.ImageCollection(
        [(testfile, 1), (testfile, 2), (testfile, 3)],
        load_func=fplug.FITSFactory)
    assert _same_ImageCollection(ic1, ic2)


@testing.skipif(not pyfits_available, reason="pyfits not installed")
def test_imread_collection_MEF_and_simple():
    io.use_plugin('fits')
    testfile1 = os.path.join(data_dir, 'multi.fits')
    testfile2 = os.path.join(data_dir, 'simple.fits')
    ic1 = io.imread_collection([testfile1, testfile2])
    ic2 = io.ImageCollection([(testfile1, 1), (testfile1, 2),
                             (testfile1, 3), (testfile2, 0)],
                             load_func=fplug.FITSFactory)
    assert _same_ImageCollection(ic1, ic2)

OK, it seems that this was implemented only for FITS support.

stefanv · 2019-07-13T16:30:38Z

Here is the relevant but extremely cryptic section of the docstring:

For files with multiple images, the images will be flattened into a list and added to the list of available images. In this case, load_func should accept the keyword argument img_num.

How does the image loader know that there are multiple frames inside of the image? Hrmmm... I feel like this functionality may need to be removed: too complex and confusing.

hmaarrfk · 2019-07-13T16:34:25Z

skimage/io/collection.py

@@ -147,24 +160,28 @@ def imread_convert(f):
    >>> coll[0].shape
    (200, 200)

-    >>> ic = io.ImageCollection('/tmp/work/*.png:/tmp/other/*.jpg')
+    >>> ic = io.ImageCollection(['/tmp/work/*.png', '/tmp/other/*.jpg'])


IMO, this should be the only supported way to setup multiple patterns in Scikit-Image 0.18.

To many options makes maintaining this kind of code super complicated.

To do this, you would have to add a deprecation warning. In that case, I see this PR more as a "simplification" of the API, since lists of strings are much more intuitive for python users than a strange seperator. I would approve ;)

As mentioned in the comments, let's just soft-deprecate the old behaviour. This means removing the documentation for the old syntax, including the second paragraph above. @stefanv do you agree with this approach?

hmaarrfk · 2019-08-25T01:43:18Z

skimage/io/collection.py

    try:
-        ar = np.concatenate(all_images)
+        array_cat = np.concatenate(all_images)


why is stack not used?

In [7]: np.stack([a, b]) Out[7]: array([[[0., 0., 0.], [0., 0., 0.], [0., 0., 0.]], [[1., 1., 1.], [1., 1., 1.], [1., 1., 1.]]]) In [8]: np.stack([a, b]).shape Out[8]: (2, 3, 3)

This really was only refactoring. I didn't think about it.

stack did not exist when this code was written.

hmaarrfk · 2019-08-25T02:07:50Z

I'm really the wrong person to ask about this PR. Personally, I don't think it should exist. imageio provides enough basic functionality. The "always sorted order" basically throws away a ton of metadata from the user. What if the user had images numbered zero through 10, but had 5 loaded images. They would never know which 5 were loaded using this function.

This function is "cute" for the examples you showed, but doesn't really extend well to any real world example I can think of where metadata is just as important for an image collection than the binary numbers themselves.

Because of the above, I don't think you will ever get a thumbs up from me. :( You shouldn't count me in as the 1/2 reviewers needed to merge this in.

That said, if you do want to push through with this, I strongly encourage you to deprecate the os.pathsep feature here, because:

I feel it is really unpythonic, adding a special character to a string in order to later turn it into a list seems like a bashism.
It puts undue complexity indefinitely in the scikit-image library.

For now, you still have a thumbs down since it seems to miss a test for the new functionality you added.

alexdesiqueira · 2019-08-25T03:21:59Z

I'm really the wrong person to ask about this PR. Personally, I don't think it should exist.
(...)
Because of the above, I don't think you will ever get a thumbs up from me. :( You shouldn't count me in as the 1/2 reviewers needed to merge this in.

What happened with this @hmaarrfk from-the-past? 😄

IMO, this should be the only supported way to setup multiple patterns in Scikit-Image 0.18.
(...)
I would approve ;)

I'll take care of the other points.

hmaarrfk · 2019-08-25T14:52:56Z

What happened with this @hmaarrfk from-the-past?

Maybe an other way to think about it is:

Loading and saving data imposes a certain permanent organization on experimental setups.

Since you don't provide a "save collection", I just don't see how this function helps anybody.

Acquisition
Saving and loading
Analysis
Visualization

Probably best to leave scikit image focused on the later two.

Whoever deals with acquisition can deal with saving and loading.

jni · 2019-08-25T20:15:35Z

@hmaarrfk none of your points provide to me compelling evidence for deprecating working functionality. I'm perfectly ok with soft-deprecating it (removing any mention on it from the docs), but I think the functionality should remain, rather than cause unnecessary churn for downstream users.

hmaarrfk · 2019-08-25T21:13:29Z

ok, dont't deprecate. A test definitely needs to be added.

stefanv · 2019-08-26T06:49:00Z

@hmaarrfk It is true that we have a policy of not implementing one-liner's on the user's behalf. But ImageCollection is quite useful, unless you want to introduce a heavier dependency such as dask for managing delayed reading.

Use case: I have a directory with 2000 images in it (this happens surprisingly frequently). I want to load these up, take a look at some of them. Yes, I can go pick out filenames individually, but often it's handy to just load the whole bunch into a collection and to then grab items from that list-like object.

So, I'd be -1 on deprecating the ImageCollection, unless we provided explicit directions in the user documentation on how to do this with, e.g., dask.

hmaarrfk · 2019-08-26T11:14:45Z

I'm not saying deprecating all of image collection. Scikit image is many voices and you should all push forward with this PR. I'm simply suggesting deprecating the behavior that uses split(os.pathsep).

alexdesiqueira · 2019-09-06T09:19:35Z

@hmaarrfk:

Analysis
Visualization
Probably best to leave scikit image focused on the later two.

I'm humbly against you on that. In my mind we're supposed to be (or, at least, offer) a nice solution from the start (which is, of course, reading the image). I'm even against our policy that @stefanv brought up:

It is true that we have a policy of not implementing one-liner's on the user's behalf.

However, as you said, we're many voices.

jni · 2019-09-06T13:20:30Z

I actually do agree with @hmaarrfk that we should "divest" skimage from IO things, and further, also visualisation things. But this is a very long term goal and should not affect us improving existing functionality and fixing bugs. I would just be very reluctant to add any IO functionality.

jni

@alexdesiqueira if you add a test, and a note in the release notes, I'm happy to merge this as is. Soft-deprecating the old pattern is optional, as that can come later at any time.

jni · 2019-09-06T13:23:56Z

skimage/io/collection.py

@@ -104,6 +121,13 @@ class ImageCollection(object):

    Notes
    -----
+    Note that files are always stored in alphanumerical order. Also note


Suggested change

Note that files are always ~~stored in alphanumerical order. Also note~~

Note that files are always returned in alphanumerical order. Also note

jni · 2019-09-06T13:25:58Z

skimage/io/collection.py

@@ -147,24 +160,28 @@ def imread_convert(f):
    >>> coll[0].shape
    (200, 200)

-    >>> ic = io.ImageCollection('/tmp/work/*.png:/tmp/other/*.jpg')
+    >>> ic = io.ImageCollection(['/tmp/work/*.png', '/tmp/other/*.jpg'])


As mentioned in the comments, let's just soft-deprecate the old behaviour. This means removing the documentation for the old syntax, including the second paragraph above. @stefanv do you agree with this approach?

jni · 2019-09-06T13:26:49Z

skimage/io/tests/test_fits.py

-       that their constituent arrays are equal.
+    """
+    Ancillary function to compare two ImageCollection objects, checking that
+    their constituent arrays are equal.


If the FITS plugin is now broken, this should definitely be noted in the release notes.

Indeed. I'll check

jni · 2019-09-06T13:28:03Z

@alexdesiqueira LOL, timing! 👋

alexdesiqueira · 2019-09-06T13:30:20Z

@jni I'm sprinting!

alexdesiqueira · 2019-09-06T13:42:31Z

I actually do agree with @hmaarrfk that we should "divest" skimage from IO things, and further, also visualisation things. But this is a very long term goal and should not affect us improving existing functionality and fixing bugs. I would just be very reluctant to add any IO functionality.

I feel I should again express my concern of we taking things away from skimage. At some point we will restrict the package to a bunch of hard-coding people, instead of all nice people around that want something somewhat easier to use and to get things done with (myself kinda included here).
But it's only my 2 BRL cents.

jni · 2019-09-06T13:46:50Z

I feel I should again express my concern of we taking things away from skimage. At some point we will restrict the package to a bunch of hard-coding people,

no no: the idea is not to stop supporting ease of use, but to separate concerns into different packages. That way, people who need only an image processing function don't need to download matplotlib, imageio, freetype, and a bunch of other things. scikit-image would retain its ease of use through its easy interoperability with the ecosystem.

Either way, I'm not advocating removing functionality, at least not for a long time. I'm just aiming to be careful to not add a bunch of extra functionality that is not precisely aligned with our vision.

stefanv · 2019-09-16T23:30:42Z

skimage/io/collection.py

-    slicing returns a new ImageCollection, *not* a view into the data.
+
+class ImageCollection(object):
+    """Load and manage a collection of image files.

    Parameters
    ----------
    load_pattern : str or list


This description is not complete.

@stefanv I thought a little and I think you're right. Let's maintain the old behavior but without the documentation, to not break stuff for a while but not encouraging it.

jni · 2019-09-30T12:26:52Z

skimage/io/collection.py

-        implementation notes below.
+    load_pattern : str or list of str
+        Pattern string or list of strings to load. The filename path can be
+        absolute or relative.


@alexdesiqueira based on my reading of the notes from the last meeting, this description should include more detail, then we can merge!

I'll point out that my approval still stands, and @stefanv could merge when ready. =)

hmaarrfk · 2019-10-01T04:46:25Z

Curious to learn how people use this! Keep me updated!

…

On Mon, Sep 30, 2019, 2:15 PM Stefan van der Walt ***@***.***> wrote: Merged #3928 <#3928> into master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3928?email_source=notifications&email_token=AAAV7GBI2MP5TTL7EL3FSPLQMJT75A5CNFSM4HPSHF6KYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOT5XM5HQ#event-2674839198>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAV7GFHWE7VWD7JNT46733QMJT75ANCNFSM4HPSHF6A> .

ImageCollection is able to receive lists of patterns

a6a4ec9

alexdesiqueira requested a review from stefanv May 24, 2019 19:45

Adding exception for fancier patterns

deae7ee

Alexandre de Siqueira added 2 commits May 24, 2019 15:25

Trying to solve the doc error

456b634

Fixing documentation

7f776c6

jni reviewed May 25, 2019

View reviewed changes

jni requested changes May 25, 2019

View reviewed changes

Some small fixes on docs

f7eadd6

alexdesiqueira requested a review from jni May 25, 2019 15:30

jni reviewed May 26, 2019

View reviewed changes

Adding suggestions

2ebd8d1

Fixing small error

47aa046

hmaarrfk reviewed Jul 13, 2019

View reviewed changes

Alexandre de Siqueira added 3 commits August 12, 2019 06:23

Small PEP8 fix

c71c222

Solving reSt documentation

91bcd29

If you cannot beat them...

83f8e6c

alexdesiqueira requested a review from hmaarrfk August 13, 2019 00:09

hmaarrfk reviewed Aug 25, 2019

View reviewed changes

Adding tests for new IC pattern behavior

72def77

jni requested changes Sep 6, 2019

View reviewed changes

jni approved these changes Sep 6, 2019

View reviewed changes

Small fix

054a675

stefanv reviewed Sep 16, 2019

View reviewed changes

Removing documentation for old behavior

c6b1872

jni reviewed Sep 30, 2019

View reviewed changes

stefanv merged commit e01560d into scikit-image:master Sep 30, 2019

alexdesiqueira deleted the improve_collections branch July 11, 2020 22:29

cosinus11 mentioned this pull request Nov 16, 2020

Bug in ImageCollection #5069

Closed

mkcor mentioned this pull request Nov 18, 2020

Restandardize handling of Multi-Image files #2815

Merged

7 tasks

	Note that files are always ~~stored in alphanumerical order. Also note~~
	Note that files are always returned in alphanumerical order. Also note

Uh oh!

ImageCollection is able to receive lists of patterns #3928

ImageCollection is able to receive lists of patterns #3928

Uh oh!

Conversation

Description

Checklist

For reviewers

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Comment last updated at 2019-09-17 00:54:07 UTC

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!