Add support for pipelines with multiple inputs #30

lschr · 2017-10-17T13:48:06Z

Implementation of the feature discussed in #29.

Still missing: - docs - tests - Python 2 compatibility

lschr · 2017-10-17T14:01:03Z

Here is a first working (at least on Python 3.6) implementation. Key features are

One can now pass multiple ancestors to the Pipeline class's __init__.
There is a new propagate_how argument (the name is, of course, up for discussion) that specifies how attributes are propagated from multiple inputs. It can either be a integer i, which causes propagation from the i-th ancestor only, or "first", or "last" (go through the list of ancestors starting with the first/last). It defaults (for now) to 0, but maybe "first" would be better.
The pipeline decorator now supports an argc argument which specifies the number of inputs. This defaults to 1 for backwards compatibility. "all" is also allowed, in which case introspection is used to determine the number of arguments to the function/class. This would be a nice default but is not backwards compatible.

Example:

@pipeline(argc="all")
def add(a1, a2):
    return a1 + a2

i1 = pims.open("img1.tif")
i2 = pims.open("img2.tif")
i12 = add(i1, i2)

lschr · 2018-01-03T14:55:54Z

Since there were no objections, I finished and tested the implementation. I'd love to see this merged.

caspervdw · 2018-01-05T13:48:35Z

Thanks a lot @lschr and sorry for the lack of attention to this project. I will review this within the next week.

caspervdw

@lschr Thanks for this fantastic generalization of the pipelines! I left some minor notes in the review, and also one use case that your code (probably) does not cover yet.

caspervdw · 2018-01-11T12:41:19Z

slicerator/__init__.py

+            it specifies the index of the ancestor (in `ancestors`). If it is
+            'first', go through all ancestors starting with the first one until
+            one is found that has the attribute. If it is 'last', go through
+            the ancestors in reverse order. Defaults to 0.


I would prefer to stick to int here and using negative integers for indexing from the end of the ancestors list. So 0 equals 'first' and -1 equals 'last'. Probably the implementation will simplify because this is normal Python list indexing.

Sorry I now understand the necessity of this implementation, please ignore my comment.

caspervdw · 2018-01-11T12:45:38Z

slicerator/__init__.py


    def __len__(self):
-        return self._ancestor.__len__()
+        return min(a.__len__() for a in self._ancestors)


Why not len(a)?

I guess because it was self._ancestor.__len__() orginally. Since I did not know why, I did not change it.

caspervdw · 2018-01-11T12:49:24Z

slicerator/__init__.py

+        If True, don't modify `func`'s doc string to say that it has been
+        made lazy. Defaults to False
+    argc : int or 'all', optional
+        Number of arguments that are relevant for the pipeline. For instance,


A bit unclear to me, may be something like 'Number of arguments that are indexed by the pipeline', and the name itself, argc, could be turned into the more comprehensive indexed_arg_count? Or ancestor_count?

I fixed the doc and went with ancestor_count to keep it reasonably short.

caspervdw · 2018-01-11T12:54:12Z

slicerator/__init__.py


    Returns
    -------
    Pipeline
        Lazy function evaluation :py:class:`Pipeline` for `func`.
    """
+    if isinstance(argc, str):


There could be a Py2/3 issue here. Maybe better: if argc == 'all':

caspervdw · 2018-01-11T12:56:54Z

slicerator/__init__.py

        else:
            # Fall back on normal behavior of func, interpreting input
            # as a single image.
-            return cls([obj], *args, **kwargs)[0]
+            return cls(*(tuple([a] for a in ancestors) + args), **kwargs)[0]


This probably doesn't cover the case of 1 indexable and 1 non-indexable ancestor?

This is (currently) not supported and also I think it would be non-trivial to implement. A user requiring such a thing could either set ancestor_count=1 (with reduced flexibility with respect to the second ancestor) or manually turn the second ancestor into a Slicerator.

I see, this is some kind of array casting. I am 👍 to make the absence of array casting explicit and not allow ancestors with unequal lengths inside a Pipeline. So that would include adding a check at the __init__ and adapting the __len__. What do you think?

caspervdw · 2018-01-11T12:57:13Z

slicerator/__init__.py

-                or isinstance(obj, Pipeline):
-            def proc_func(x):
-                return func(x, *args, **kwargs)
+    if isinstance(argc, str):


Same as above

and update documentation

caspervdw · 2018-01-15T19:30:20Z

@lschr See my line comment. @danielballan do you want to check this as well before merging?

lschr · 2018-01-16T08:38:32Z

@caspervdw I think you are right. Let the user deal with differently-sized ancestors himself since he is the only one knowing how to do so properly anyways. Pipeline.__init__ will now raise a ValueError if ancestors' sizes don't match.

caspervdw · 2018-01-18T16:01:59Z

Thanks @lschr, good to merge from my side. @danielballan ?

danielballan · 2018-01-19T18:28:27Z

I would like to look at this before merging. Swamped today, will try to look this weekend.

danielballan · 2018-02-08T21:15:11Z

I haven't had a chance to look at this, but I don't want to keep blocking progress, so I will trust @caspervdw and push the green button. Sorry for the hold-up!

Add support for pipelines with multiple inputs

f006fdb

Still missing: - docs - tests - Python 2 compatibility

lschr mentioned this pull request Oct 17, 2017

Pipelines with multiple inputs #29

Open

Lukas Schrangl added 8 commits January 3, 2018 11:38

Fix Python 3.4

2e24695

Fix for Python 2.7

d99961f

Document usage of pipelines with multiple inputs

1c79e58

Fix Pipeline with propagate_how="last"

22e7075

Add tests for multiple inputs to pipelines

4d8173d

Fix Pipeline for multiple inputs which are not Slicerators

a2e56c9

Add more attribute propagation tests

ee7a45e

Make 'first' the default for propagate_how in Pipeline

ed45df3

lschr changed the title ~~Add support for pipelines with multiple inputs (in progress)~~ Add support for pipelines with multiple inputs Jan 3, 2018

caspervdw requested changes Jan 11, 2018

View reviewed changes

Lukas Schrangl added 3 commits January 11, 2018 15:51

Update docstring for ed45df3

7126b16

Rename argc -> ancestor_count in pipeline

44b025a

and update documentation

Make argument checks Python 2 safe in pipeline

615bf79

caspervdw approved these changes Jan 15, 2018

View reviewed changes

Allow only ancestors of same length in Pipeline

06fc2fe

danielballan merged commit bd9e1b4 into soft-matter:master Feb 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for pipelines with multiple inputs #30

Add support for pipelines with multiple inputs #30

lschr commented Oct 17, 2017 •

edited

Loading

lschr commented Oct 17, 2017 •

edited

Loading

lschr commented Jan 3, 2018

caspervdw commented Jan 5, 2018

caspervdw left a comment

caspervdw Jan 11, 2018

caspervdw Jan 11, 2018

caspervdw Jan 11, 2018

lschr Jan 11, 2018

caspervdw Jan 11, 2018

lschr Jan 11, 2018

caspervdw Jan 11, 2018

lschr Jan 11, 2018

caspervdw Jan 11, 2018

lschr Jan 11, 2018

caspervdw Jan 15, 2018

caspervdw Jan 11, 2018

lschr Jan 11, 2018

caspervdw commented Jan 15, 2018

lschr commented Jan 16, 2018

caspervdw commented Jan 18, 2018

danielballan commented Jan 19, 2018

danielballan commented Feb 8, 2018

Add support for pipelines with multiple inputs #30

Add support for pipelines with multiple inputs #30

Conversation

lschr commented Oct 17, 2017 • edited Loading

lschr commented Oct 17, 2017 • edited Loading

lschr commented Jan 3, 2018

caspervdw commented Jan 5, 2018

caspervdw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caspervdw commented Jan 15, 2018

lschr commented Jan 16, 2018

caspervdw commented Jan 18, 2018

danielballan commented Jan 19, 2018

danielballan commented Feb 8, 2018

lschr commented Oct 17, 2017 •

edited

Loading

lschr commented Oct 17, 2017 •

edited

Loading