Piecewise param #1355

AllenHW · 2017-09-11T19:26:41Z

Motivation and context:
piecewise() function accepts a dictionary of time points and corresponding function/constants. We want to turn this into be a process. Thus we need a new parameter type(s) used to define a piecewise function.

Previous PR used two NdArrays, tp and yp to define a piecewise function. This limits users from defining a piecewise function that includes non-constant subdomains (i.e. sin() from 0.5 < t < 1). The following are two options I considered:

Option 1: use NdArray for tp, and add a parameter NumericalSeries, which represents a list of either numerical constants OR callable functions whose output dimentions are the same
Option 2 (chosen): Add a dictionary parameter who keys are time points and values are either callable functions or numerical constants

Both parameters would be specific to piecewise process. I chose option 2 to maintain previous interface of piecewise() function

Interactions with other PRs:
This change includes changes from #1100. Addition of PiecewiseDataParam is included.

How has this been tested?
Added new unit tests andran them

How long should this take to review?

Average (neither quick nor lengthy)

Where should a reviewer start?
Please have a look at the first PR. Then, have a look at PiecewiseDataParam to see if the logic makes sense, as well as the choice of such parameter type.

Types of changes:

New feature (non-breaking change which adds functionality)

Checklist:

I have read the CONTRIBUTING.rst document.
I have updated the documentation accordingly.
I have included a changelog entry.
I have added tests to cover my changes.
All new and existing tests passed.

Still to do:

tbekolay · 2017-09-14T15:26:37Z

I'm 👍 on the param in general; it's not very long so it doesn't add much extra complexity.

tbekolay

The main content of this PR is great, I think storing the data dict in new param makes sense and simplifies the implementation! There are a few stylistic things that I've commented about inline that need to be addressed though. Additionally, it would be good to rebase this branch to master, as the Piecewise process has changed a bit in master compared to where this branch started.

tbekolay · 2017-09-29T14:51:19Z

nengo/params.py

@@ -350,6 +350,35 @@ def coerce(self, instance, value):
        return super(DictParam, self).coerce(instance, value)


+class PiecewiseDataParam(DictParam):


Can you put this class in processes.py instead of params.py? This is what we normally do with params that apply only to a single class (e.g., PrePostParam in connection.py).

tbekolay · 2017-09-29T14:59:31Z

nengo/params.py

+    def coerce(self, instance, data):
+        data = super(PiecewiseData, self).coerce(instance, data)
+
+        self.interpolatable = True


It makes sense to keep track of this in the param, rather than in the piecewise class. However, for clarity, we define all instance variables in the __init__ method, even though there's no other reason to implement __init__ here (though you could additionally make the constructor easier by fixing some parameters like readonly).

So yeah, define this in __init__ (probably defaulting to False?) and then change it in this method (which I think is not done at the moment).

tbekolay · 2017-09-29T15:02:24Z

nengo/params.py

+        for time, value in data.iteritems():
+            if not is_number(time):
+                raise ValidationError("Keys must be times (floats or ints), not %r"
+                                      % type(time).__name__, attr='data')


Should also pass obj=instance here.

tbekolay · 2017-09-29T15:02:45Z

nengo/params.py

+            # make sure this is the same length as previous items
+            if length != output_length and output_length is not None:
+                raise ValidationError("time %g has %d items instead of %d" %
+                                      (time, length, output_length), attr='data')


obj=instance here too

tbekolay · 2017-09-29T15:03:04Z

nengo/processes.py

@@ -4,7 +4,7 @@
 from nengo.base import Process
 from nengo.dists import DistributionParam, Gaussian
 from nengo.exceptions import ValidationError
-from nengo.params import BoolParam, DictParam, NdarrayParam, NumberParam
+from nengo.params import BoolParam, DictParam, EnumParam, NdarrayParam, NumberParam, PiecewiseDataParam


This line is too long, should be < 80 characters.

tbekolay · 2017-09-29T15:08:19Z

nengo/processes.py

+                return False
+        return True
+
+    def _output_length(self):


This could also be a property of the param

tbekolay · 2017-09-29T15:09:32Z

nengo/processes.py

+
+    Parameters
+    ----------
+    tp : time points of the function where values are specified


This will have to be updated, as tp and yp are no longer attributes of the param.

tbekolay · 2017-09-29T15:10:42Z

nengo/tests/test_processes.py

+    def test_function(self, Simulator):
+        f = self.run_sim({0: np.sin, 0.5: np.cos}, 'zero', Simulator)
+
+        assert np.allclose(f[0], [np.sin(0.001)])   # t = 0.001


As I did in the other other tests, we should opt for passing in things like t == 0.001 rather than passing in 0 and commenting the actual time (which is more understandable to the reader).

tbekolay · 2017-09-29T15:19:04Z

nengo/tests/test_processes.py

+        assert np.allclose(f[999], [np.cos(1.0)])   # t = 1.0
+        assert np.allclose(f[1499], [np.cos(1.5)])  # t = 1.5
+
+


Inside a class, PEP8 says to leave one blank line between methods, not more than one.

tbekolay · 2017-09-29T15:19:30Z

nengo/tests/test_processes.py

+        assert np.allclose(f[499], [1]) # t = 0.5
+        assert np.allclose(f[749], [0.5]) # t = 0.75
+        assert np.allclose(f[999], [0]) # t = 1.0
+


Additionally, PEP8 says no extra newlines at the end of a file.

AllenHW · 2017-10-04T01:16:42Z

nengo/processes.py

-            default_size_in=0, default_size_out=self.yp[0].size, **kwargs)
+            default_size_in=0, default_size_out=self._shape_out()[0], **kwargs)
+
+    def _interpolatable(self):


I had initially thought of tracking this in the PiecewiseDataParam class, but didn't know if we could retract any info from the descriptor class.

Ah you're right, it is difficult to access attributes of descriptor classes. Never mind!

tbekolay

Pushed a commit, mostly to remove the underscored methods. With this, LGTM!

Note that there were some issues with the rebase; specifically, master contains a modified version of da80a22, cfe51f3, and 7c5206a so those commits should not have been included post-rebase. I'll fix this in the merge (which will be easy because most of the issues were fixed post-rebase), but just wanted to point it out for future rebasing.

drasmuss

Pushed a commit with some minor suggestions:

np.ravel(...) will implicitly cast its arguments to np
arrays if necessary, so we can be a bit more
parsimonious than np.asarray(...).ravel()
The size_out property seemed like it was doing a lot
of work to compute the size out, so I was trying to
think of ways to simplify it. I'm ambivalent about this
solution; it's nice and simple, but a bit weird to have
parameters be setting attributes on their instance. I'd
be fine if people wanted to reject this change and
just leave it the way it was, just putting it out there as
an option.

tbekolay · 2017-10-05T17:06:12Z

I like the ravel change, and setting a property on the instance is done in some Nengo objects... but I think if we're going to set it on the instance, we should add a size_out param to it and some documentation that it will get set automatically, which seems like more effort than keeping the property. While it does seem that the property does a lot, it's really only making an iterator (should be very cheap) and getting the first item (also cheap) so I'ma revert that one, though it would have been nice to not recompute the size_out.

In doing so, I also added the PiecewiseDataParam class, rather than stripping out tp and yp when the process is made.

AllenHW · 2017-10-05T17:35:30Z

Thanks for the changes! It looks neater now 😄

Didn't know we could modify the instance from the descriptor....that's actually pretty cool. I was also thinking it'd be to nice to compute things once in the descriptor, but I'll go along with @tbekolay

tbekolay · 2017-10-05T17:56:36Z

I'll go along with @tbekolay

Seanny123 added the work in progress label Sep 14, 2017

tbekolay requested changes Sep 29, 2017

View reviewed changes

AllenHW force-pushed the piecewise-param branch 2 times, most recently from 8e2137f to 47af365 Compare October 4, 2017 01:14

AllenHW commented Oct 4, 2017

View reviewed changes

tbekolay approved these changes Oct 5, 2017

View reviewed changes

tbekolay requested a review from drasmuss October 5, 2017 15:25

drasmuss approved these changes Oct 5, 2017

View reviewed changes

Handle callables in Piecewise process

b44a1f5

In doing so, I also added the PiecewiseDataParam class, rather than stripping out tp and yp when the process is made.

tbekolay force-pushed the piecewise-param branch from 9e2b946 to b44a1f5 Compare October 5, 2017 17:33

tbekolay merged commit b44a1f5 into master Oct 5, 2017

tbekolay deleted the piecewise-param branch October 5, 2017 17:58

tbekolay mentioned this pull request Oct 6, 2017

Release v1.3.0 nengo/nengo-ocl#148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Piecewise param #1355

Piecewise param #1355

AllenHW commented Sep 11, 2017 •

edited

Loading

tbekolay commented Sep 14, 2017

tbekolay left a comment •

edited

Loading

tbekolay Sep 29, 2017

tbekolay Sep 29, 2017

tbekolay Sep 29, 2017

tbekolay Sep 29, 2017

tbekolay Sep 29, 2017

tbekolay Sep 29, 2017

tbekolay Sep 29, 2017

tbekolay Sep 29, 2017

tbekolay Sep 29, 2017

tbekolay Sep 29, 2017

AllenHW Oct 4, 2017

tbekolay Oct 4, 2017

tbekolay left a comment

drasmuss left a comment

tbekolay commented Oct 5, 2017

AllenHW commented Oct 5, 2017

tbekolay commented Oct 5, 2017

		@@ -350,6 +350,35 @@ def coerce(self, instance, value):
		return super(DictParam, self).coerce(instance, value)


		class PiecewiseDataParam(DictParam):

		assert np.allclose(f[999], [np.cos(1.0)]) # t = 1.0
		assert np.allclose(f[1499], [np.cos(1.5)]) # t = 1.5

Piecewise param #1355

Piecewise param #1355

Conversation

AllenHW commented Sep 11, 2017 • edited Loading

tbekolay commented Sep 14, 2017

tbekolay left a comment • edited Loading

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

tbekolay left a comment

Choose a reason for hiding this comment

drasmuss left a comment

Choose a reason for hiding this comment

tbekolay commented Oct 5, 2017

AllenHW commented Oct 5, 2017

tbekolay commented Oct 5, 2017

AllenHW commented Sep 11, 2017 •

edited

Loading

tbekolay left a comment •

edited

Loading