Connections can take points for functions #1010

hunse · 2016-04-11T19:13:41Z

When specifying a Connection function, points can now be passed to
implicitly define the function along with the eval points. This
addresses #323.

arvoelke · 2016-04-11T19:39:00Z

nengo/connection.py

+        else:
+            raise ValidationError("Invalid connection function type %r "
+                                  "(must be callable or array-like)"
+                                  % function.__class__.__name__,


If the function failed the above checks then it might not have a class name attribute.

I was assuming that everything has a __class__ attribute, but I don't think that's quite true. Better to use type() instead? There are probably other places we make this assumption, too (e.g. node outputs).

I think there are only a couple rare cases in Python 2 where this attribute won't exist. I'm not really up to snuff on Python 3 so I'm afraid I don't know what the different cases are, or what the best suggestion would be. type does sound more reliable, but again I don't know.

Edit: It may be that the only case in Python 2 is with old-style classes, and so this is fine in Python 3. I can't offer any sources to substantiate this unfortunately.

Ok, well I fixed it everywhere anyway.

hunse · 2016-05-24T20:50:55Z

Okay, fixed this up. The first three commits are fixes to the code that help with the final commit, which actually allows function points to be passed to connections.

The first three commits can be separated out and merged independently if there are questions about the fourth. I'd like to get them in ASAP, since the second one in particular touches large amounts of the codebase and is annoying to rebase.

Seanny123 · 2016-05-24T21:57:04Z

nengo/connection.py

        Function to compute across the connection. Note that ``pre`` must be
        an ensemble to apply a function across the connection.
+        If an array is passed, the function is implicitly defined by the
+        array points and the provided ``eval_points``.


Maybe say "the array points referenced" since you explicitly don't copy them?

Also maybe mention that the array has to be 2D?

It gives the required shape above. Does that make it clear that it's 2D?

If I read "array points referenced" I'm not sure I would think that means they're passed by reference, i.e. not copied. Also, I think the assumption is that anything passed as a parameter is not copied, so if you pass an array and then change a value it will change that in your model. Of course, when everything is built, we make copies as needed so that the built model would never be changed.

It gives the required shape above. Does that make it clear that it's 2D?

Aw jeez, I totally missed that notation.

I think the assumption is that anything passed as a parameter is not copied

That's true. I forgot about that assumption when it comes to Python.

Seanny123 · 2016-05-24T22:01:36Z

I did my best at reviewing. Other than some documentation nitpicks, it LGTM.

hunse · 2016-06-07T18:46:28Z

Could this be looked at soon? It would be helpful for my tutorial.

jgosmann · 2016-06-07T19:00:55Z

nengo/tests/test_node.py

    with nengo.Network() as model:
+        # type errors


This comment is misleading. The next few lines are perfectly fine and don't produce type errors. (Took me a few moments to realize why there is no check for raised exceptions for this lines.)

The last line of this section is a "type" error (i.e. the node output is not an acceptable type, not literally a TypeError, though). But you're right, this is mostly checking that the supported types work fine. Would you prefer no comment? Or a different comment?

Maybe move the comment down to where actually a type error occurs and change this line to something like # valid values?

Yeah, okay.

jgosmann · 2016-06-07T19:16:09Z

Comments aside LGTM 🍰

tbekolay · 2016-06-07T19:49:54Z

This looks good to me too! The speed concern I think outweighs the concern of making the function kwarg too complex... and we have validation now so that eval_points have to be set first, so surprise should be minimized.

I made two small fixup commits to reword some strings and add a deprecation warning to nengo.utils.connection.target_function (which we implemented as a helper in lieu of doing this previously). I think in 2.2 we should switch target_function to raise a DeprecationError, then someday remove it entirely; I noted this in #822.

This also needs a changelog entry. If my changes look good to @hunse I'll merge when a changelog entry is pushed!

tbekolay · 2016-06-07T19:56:42Z

Oh also, this is a relatively big API change and we've already done several minor bugfixes; once this is merged, should we cut a 2.1.1 release? My thinking is yes to start getting in the habit of more frequent, smaller releases.

jgosmann · 2016-06-07T20:05:58Z

+1 for more releases (especially timely bugfix releases)
Though, it is not clear to me what decides whether we do a patch level or minor release. According to semantic versioning: "MINOR version when you add functionality in a backwards-compatible manner". This clearly adds functionality, so shouldn't this be 2.2?

hunse · 2016-06-07T20:09:51Z

"MINOR version when you add functionality in a backwards-compatible manner". This clearly adds functionality, so shouldn't this be 2.2?

I wouldn't be too strict about that. Especially if we want to do more releases (which I agree with), if we do minor increments for stuff like this we'll be at 2.100.0 before we know it, and that just looks ridiculous.

Do we want to do that mini-sprint we were talking about before the release, @tbekolay? I'd be fine releasing after this, but I think there are also a number of things waiting that we might as well get merged and released.

tbekolay · 2016-06-07T20:10:08Z

This clearly adds functionality, so shouldn't this be 2.2?

Right you are! And actually, the switch in the LIF model is also probably 2.2 worthy. Any objections to that?

hunse · 2016-06-07T20:11:47Z

If we're going to go to 2.2, I'd try to get a few more of the waiting PRs in.

jgosmann · 2016-06-07T20:12:49Z

I wouldn't be too strict about that.

Yes, but at the moment it looks pretty random to me which makes the versioning scheme somewhat useless.

We could also do a 2.1.1 bugfix release with what's been merged (and maybe a few other small bugfix PRs) and then do this and more in a 2.2 release.

tbekolay · 2016-06-07T20:21:19Z

if we do minor increments for stuff like this we'll be at 2.100.0

Indeed, Linux switched arbitrarily from 2.6.39 to 3.0 because the .39 was just too much. I think we will eventually want to do API-breaking changes, so we shouldn't be that opposed to bumping up to 3 sometime.

could also do a 2.1.1 bugfix release with what's been merged (and maybe a few other small bugfix PRs)

This crossed my mind also. My thoughts for other small bugfix PRs to merge for 2.1.1 (in no particular order): #1074, #1027, #1005 (if that's still valid)

jgosmann · 2016-06-07T20:24:44Z

Replied in #1005 regarding validity.

The message used to use `node.output`, which at that point was still unset. Now uses `output`.

Rather than using `<obj>.__class__`, we now use `type(<obj>)`, since this is safer for some old types of objects.

``size_in``, ``size_mid``, and ``size_out`` are now well defined, and used properly when identifying function and transform sizes.

tbekolay · 2016-07-29T19:35:34Z

I rebased this and fixed up the fixups. It's been reviewed already, but I'm going to give people the weekend to give this a look-over as it's a relatively big change; I'll merge Monday morning if no issues come up.

tbekolay · 2016-08-01T17:13:26Z

Pushed a fixup to change a nengo.Simulator in a test to Simulator, and one to add a changelog entry. I'll merge once the tests finish.

When specifying a Connection function, points can now be passed to implicitly define the function along with the eval points. This addresses #323.

hunse added the needs review label Apr 11, 2016

arvoelke reviewed Apr 11, 2016
View reviewed changes

hunse force-pushed the function-points branch from 525e7a1 to 320aee8 Compare April 12, 2016 21:01

Seanny123 added this to the 2.1.1 release milestone Apr 22, 2016

hunse force-pushed the function-points branch from 00a985e to c5c6c55 Compare May 24, 2016 20:43

Seanny123 reviewed May 24, 2016
View reviewed changes

Seanny123 added reviewed and removed needs review labels May 24, 2016

tbekolay self-assigned this May 26, 2016

hunse force-pushed the function-points branch from c5c6c55 to 7e0db93 Compare June 7, 2016 18:46

jgosmann self-assigned this Jun 7, 2016

jgosmann reviewed Jun 7, 2016
View reviewed changes

hunse force-pushed the function-points branch from 7e0db93 to 5c8bbaa Compare June 7, 2016 19:30

tbekolay added this to the 2.2.0 release milestone Jun 7, 2016

tbekolay removed this from the 2.1.1 release milestone Jun 7, 2016

tbekolay mentioned this pull request Jun 7, 2016

2.1.1, 2.2.0 releases #1080

Closed

12 tasks

jgosmann assigned tbekolay and unassigned tbekolay and jgosmann Jun 8, 2016

Seanny123 added reviewed and removed needs review labels Jun 10, 2016

hunse mentioned this pull request Jun 21, 2016

NEF vision nengo/nengo-extras#14

Merged

tbekolay removed their assignment Jul 29, 2016

hunse added 3 commits July 29, 2016 15:24

Fix error message for wrong node output type

8b167d5

The message used to use `node.output`, which at that point was still unset. Now uses `output`.

Prefer type to __class__

76f0a34

Rather than using `<obj>.__class__`, we now use `type(<obj>)`, since this is safer for some old types of objects.

Clarified size documentation in Connection

1d7c0a2

``size_in``, ``size_mid``, and ``size_out`` are now well defined, and used properly when identifying function and transform sizes.

tbekolay force-pushed the function-points branch from db8788f to 3f00c04 Compare July 29, 2016 19:33

tbekolay force-pushed the function-points branch from 8522f9b to 58d65f3 Compare August 1, 2016 17:16

Connections can take points for functions

5be289d

When specifying a Connection function, points can now be passed to implicitly define the function along with the eval points. This addresses #323.

tbekolay force-pushed the function-points branch from 58d65f3 to 5be289d Compare August 1, 2016 17:28

tbekolay merged commit 5be289d into master Aug 1, 2016

tbekolay deleted the function-points branch August 1, 2016 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connections can take points for functions #1010

Connections can take points for functions #1010

hunse commented Apr 11, 2016

arvoelke Apr 11, 2016

hunse Apr 11, 2016

arvoelke Apr 11, 2016

hunse Apr 11, 2016

hunse commented May 24, 2016

Seanny123 May 24, 2016

Seanny123 May 24, 2016

hunse May 25, 2016

Seanny123 May 25, 2016

Seanny123 commented May 24, 2016

hunse commented Jun 7, 2016

jgosmann Jun 7, 2016

hunse Jun 7, 2016

jgosmann Jun 7, 2016

hunse Jun 7, 2016

jgosmann commented Jun 7, 2016

tbekolay commented Jun 7, 2016

tbekolay commented Jun 7, 2016 •

edited

Loading

jgosmann commented Jun 7, 2016 •

edited

Loading

hunse commented Jun 7, 2016

tbekolay commented Jun 7, 2016

hunse commented Jun 7, 2016

jgosmann commented Jun 7, 2016

tbekolay commented Jun 7, 2016

jgosmann commented Jun 7, 2016

tbekolay commented Jul 29, 2016

tbekolay commented Aug 1, 2016

Connections can take points for functions #1010

Connections can take points for functions #1010

Conversation

hunse commented Apr 11, 2016

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

hunse commented May 24, 2016

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

Seanny123 commented May 24, 2016

hunse commented Jun 7, 2016

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

jgosmann commented Jun 7, 2016

tbekolay commented Jun 7, 2016

tbekolay commented Jun 7, 2016 • edited Loading

jgosmann commented Jun 7, 2016 • edited Loading

hunse commented Jun 7, 2016

tbekolay commented Jun 7, 2016

hunse commented Jun 7, 2016

jgosmann commented Jun 7, 2016

tbekolay commented Jun 7, 2016

jgosmann commented Jun 7, 2016

tbekolay commented Jul 29, 2016

tbekolay commented Aug 1, 2016

tbekolay commented Jun 7, 2016 •

edited

Loading

jgosmann commented Jun 7, 2016 •

edited

Loading