Add Piecewise process #1100

hunse · 2016-06-17T21:37:03Z

Description:
Turns the piecewise function into a Process. This adds all the normal amenities, such as running the process for a length of time outside a Nengo simulator.

Motivation and context:

Addresses issue #1036. It should make this easier to use.

I'm also adding interpolation, so that we can do e.g. linear interpolation rather than just the current zero-order hold behaviour.

If we really want to go the whole nine yards, we could actually allow this to take other Processes in addition to callables and constants, so that we can e.g. run one process for 5 seconds, then another for the next 3 seconds, etc.

How has this been tested?

Still TODO. I'm planning on pulling over all the tests from the old piecewise function, so we maintain all that behaviour.

Where should a reviewer start?

Look at the new process first, and read the documentation (still TODO).

How long should this take to review?

still unknown

Quick (less than 40 lines changed or changes are straightforward)
Average (neither quick nor lengthy)
Lengthy (more than 150 lines changed or changes are complicated)

Types of changes:

Breaking change (fix or feature that causes existing functionality to change)

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:

Support callables as outputs
Support Processes as outputs if we want to do that (would it be useful?)
Update old tests so they work with the new Process
Test interpolation
Update examples that use old piecewise
Remove old piecewise function and tests, or perhaps just add a deprecation warning.

mention-bot · 2016-06-17T21:37:05Z

By analyzing the blame information on this pull request, we identified @tbekolay to be a potential reviewer

AllenHW · 2017-09-08T18:50:41Z

After discussing with @hunse, we decided to support callable in another PR. To do it we would require another parameter type that lets user pass an array of callable AND floats/list of floats

tbekolay

Overall this is looking great, thanks @AllenHW!

I've made some inline comments here for you to address. Normally in a review we would make commits for the changes, but because this is your first PR I thought it would be better to provide more justifications for the changes requested and to have you make them. Later on (after a few accepted PRs), I'll make the changes myself, as it tends to lower the amount of back-and-forth needed for each PR.

Additionally, this PR is currently failing unit tests; check out the TravisCI build for details, but also it's helpful to run the tests locally because pushing and waiting for TravisCI is almost always slower than running the tests locally.

tbekolay · 2017-09-09T13:21:23Z

CHANGES.rst

+**Changed**
+
+- piecewise() function is now a process. Support for interpolation of
+points was also added


restructedText is pretty strict about some things, one of which being that this second line must be indented to the same level as "piecewise" above. So this line needs some whitespace at the beginning.

Also, we always (manually) add a link to the pull request at the end of the changelog entry; see the other entries in this file for examples.

tbekolay · 2017-09-09T13:28:33Z

nengo/processes.py

+class Piecewise(Process):
+    """Present a piecewise input with different options for interpolation.
+
+    Given an input of tp = [0, 0.5, 0.75, 1], yp = [[0], [1], [-1], [0]] this will generate a


With very few exceptions, we stick to a strict line-length limit of 79 characters, as espoused by PEP8 and enforced by flake8. If possible, it would be great to configure your text editor or IDE to run the flake8 checks on Python files as you're editing them! Most editors can be configured to do this if you google around a bit.

Also, any text that corresponds to code we usually render in a monospaced font, which is done in docstrings and other restructedText snippets with two backticks before and after the code. I would probably do that for tp and yp here.

tbekolay · 2017-09-09T13:36:07Z

nengo/processes.py

+    Elements in tp must be times (floats or ints). The values in yp can be floats for
+    1d or lists for multi-dimensional function. All lists must be of the same length.
+
+    Interpolations on the data points using scipy.interpolation are also supported. The default


"Interpolations" -> "Interpolation"
"are" -> "is"

tbekolay · 2017-09-09T13:43:50Z

nengo/processes.py

+
+    Interpolations on the data points using scipy.interpolation are also supported. The default
+    interpolation is 'zero', which simply creates a piecewise function whose values begin
+    at the specified time points. So the above example would be shortcut for:


To render the below snippet as a source code snippet properly, you have to change the colon at the end of this line do a double colon (... would be a shortcut for::) and add a newline separating this paragraph from the code snippet.

tbekolay · 2017-09-09T13:45:40Z

nengo/processes.py

+    For times before the first specified time, it will default to zero (of
+    the correct length). This means that the above can be simplified to:
+
+        Piecewise([0.5, 0.75, 1], yp = [[1], [-1], [0]])


When passing arguments to a function or class, PEP8 tells us not put spaces around the =.

Additionally, this doesn't seem to match with the current __init__ for this class, which accepts a data dictionary; the tp and yp are extracted from the data dictionary. Was your intention to change what gets passed to Piecewise?

tbekolay · 2017-09-09T14:02:46Z

nengo/processes.py

+                import scipy.interpolate
+                self.sp_interpolate = scipy.interpolate
+            except ImportError:
+                    raise ValidationError(


This block is overindented by 4 spaces; it should line up with the try block above.

Also, I think this should raise an ImportError rather than a ValidationError.

I am also wondering if, rather than raising an error, we should instead raise a warning and set interpolate to 'zero' instead of crashing. What do you think?

tbekolay · 2017-09-09T14:13:25Z

nengo/tests/test_processes.py

+
+class TestPiecewise(object):
+
+    dt = 0.001


Unless there's some reason to change the dt in a specific test, it's probably best not to specify it, as specifying it indicates to the reader that we're going to be using a non-standard dt in some tests (which I don't believe we are here).

tbekolay · 2017-09-09T14:15:49Z

nengo/tests/test_processes.py

+            process = nengo.processes.Piecewise(data, 'cubic')
+            assert process
+
+    def test_interpolation(self, Simulator):


At the moment, this test requires scipy, which is not a required dependency of Nengo. In order to not fail unit tests when scipy is not installed, please use pytest.importorskip (see test_dists.py for an example).

tbekolay · 2017-09-09T14:16:31Z

nengo/utils/functions.py

 def piecewise(data):
+    warnings.warn("piecewise() function is deprecated. Use Piecewise process instead.",
+                      DeprecationWarning)


This line is overindented, according to PEP8. It should line up with the p in piecewise.

tbekolay · 2017-09-09T14:17:46Z

nengo/utils/tests/test_functions_piecewise.py

@@ -4,6 +4,10 @@
 from nengo.exceptions import ValidationError
 from nengo.utils.functions import piecewise

+"""
+note:: piecewise() function is deprecated. Use Piecewise process instead.
+"""


This note isn't necessary, as it's very unlikely anyone will be touching this test file. We raise the deprecation warning in utils/functions.py already, which is something that will be seen when running these tests anyhow.

tbekolay

Thanks for the fixes @AllenHW! There were a few lingering issues, but since there weren't many I went ahead and made the changes in commits which I've pushed here. Please look them over @AllenHW and make sure they look good to you.

hunse · 2017-09-15T19:03:13Z

I looked it over, and it all looks great to me! Thanks @AllenHW!

I added a little fixup to use interpolation as a keyword argument only, just because I'm nit-picky like that sometimes.

drasmuss

Pushed a small fixup commit. Mainly minor documentation/typo fixes. The only substantive changes are

I removed a validation check that wasn't necessary (no way for that validation to fail)
I added a check that all the Piecewise values have the same length (previously this would just fail when
you tried to create an ndarray with mismatched values, which wasn't very informative)

If those changes look good then this looks good to me.

drasmuss · 2017-09-15T19:23:22Z

nengo/processes.py

+            raise ValidationError(
+                "`tp.shape[0]` (%d) must equal `yp.shape[0]` (%d)"
+                % (self.tp.shape[0], self.yp.shape[0]),
+                attr='yp', obj=self)


There is no way this ValidationError could ever trigger (since a dictionary always has the same number of keys and values).

tbekolay · 2017-09-20T15:27:15Z

Those changes look good to me @drasmuss! Can you approve this so I can merge it in?

And deprecate the old piecewise function. Also updated all tests and examples to use the new process. Currently we do not yet deal with callables. There are no examples that use callables, but regardless we should implement that functionality and test it. Fixes #1036. Co-authored by: Allen Wang <allen.houze.wang@gmail.com>

drasmuss added the work in progress label Jun 27, 2016

tbekolay mentioned this pull request May 10, 2017

nengo.utils and nengo_extras #1301

Closed

AllenHW force-pushed the piecewise-process branch from 601f07b to 12ccab1 Compare September 8, 2017 18:21

nengo deleted a comment from codecov-io Sep 9, 2017

tbekolay requested changes Sep 9, 2017

View reviewed changes

AllenHW mentioned this pull request Sep 11, 2017

Piecewise param #1355

Merged

5 tasks

tbekolay changed the title ~~WIP: Add Piecewise process~~ Add Piecewise process Sep 15, 2017

tbekolay approved these changes Sep 15, 2017

View reviewed changes

drasmuss reviewed Sep 15, 2017

View reviewed changes

drasmuss approved these changes Sep 20, 2017

View reviewed changes

tbekolay force-pushed the piecewise-process branch from cd75c6c to b627245 Compare September 21, 2017 14:06

tbekolay force-pushed the piecewise-process branch from b627245 to 736a92a Compare September 21, 2017 19:18

tbekolay merged commit 736a92a into master Sep 21, 2017

tbekolay deleted the piecewise-process branch September 21, 2017 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Piecewise process #1100

Add Piecewise process #1100

hunse commented Jun 17, 2016 •

edited by AllenHW

Loading

mention-bot commented Jun 17, 2016

AllenHW commented Sep 8, 2017

tbekolay left a comment •

edited

Loading

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay Sep 9, 2017

tbekolay left a comment

hunse commented Sep 15, 2017

drasmuss left a comment

drasmuss Sep 15, 2017

tbekolay commented Sep 20, 2017

Add Piecewise process #1100

Add Piecewise process #1100

Conversation

hunse commented Jun 17, 2016 • edited by AllenHW Loading

mention-bot commented Jun 17, 2016

AllenHW commented Sep 8, 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

hunse commented Sep 15, 2017

drasmuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbekolay commented Sep 20, 2017

hunse commented Jun 17, 2016 •

edited by AllenHW

Loading

tbekolay left a comment •

edited

Loading