Add EnsembleArray.add_neuron_input/output. #868

tbekolay · 2015-10-04T15:03:55Z

Previously, these both happened whenever neuron_nodes=True was passed in. However, there are occasions when you only want a neuron input (as occurred in InputGatedMemory) or only want a neuron output (for plotting a raster, for example). Additionally, adding these functions makes the __init__ function a bit more readable.

For the time being, I've kept neuron_nodes around as a shortcut to calling add_neuron_input and add_neuron_output. However, I've added a DeprecationWarning when it's set to True; we should remember to remove neuron_nodes in Nengo 2.2.

I also redid the unit tests appropriately, and moved the EA.dimensions property up in the file, as I tend to expect all properties to be organized before all methods.

hunse · 2015-10-04T16:25:20Z

What if we just had neuron_input and neuron_output be properties. When they're called the first time, they'll create a new node, otherwise they'll just return the existing one. That way, if someone uses neuron_input or neuron_output in the model, they'll be created, otherwise they won't.

tbekolay · 2015-10-04T18:36:39Z

Could do it that way. Just seems a bit magical, but maybe in a good way?

tcstewar · 2015-10-04T19:05:25Z

What if we just had neuron_input and neuron_output be properties. When they're called the first time, they'll create a new node, otherwise they'll just return the existing one. That way, if someone uses neuron_input or neuron_output in the model, they'll be created, otherwise they won't.

I'll vote against that approach. I did it that way in spa for some bias nodes, and the problem is that when the GUI system starts looking inside an object for properties, it references those, causing them to be created. So at some point I'll be going in and fixing the bias system to not do that. But I think doing it as a side effect of just accessing the variable is a bit too magical, especially given all the introspection stuff that's possible/encouraged in Python.

arvoelke · 2015-10-05T15:02:12Z

nengo/networks/ensemblearray.py

+            warnings.warn(
+                "'neuron_nodes' argument will be removed in Nengo 2.2. Use "
+                "'add_neuron_input' and 'add_neuron_output' methods instead.",
+                DeprecationWarning)


Add to #822.

arvoelke · 2015-10-05T15:05:54Z

My only concern is that add_output can be called multiple times, while add_neuron_input and add_neuron_output can only be called once. This limitation seems a little arbitrary/inconsistent to the end-user. I'd be more comfortable if they could both be called multiple times, or moved into __init__.

hunse · 2015-10-05T15:29:51Z

I really don't think we want add_neuron_input/output being callable multiple times. Then you'd end up with multiple nodes doing exactly the same thing, slowing stuff down.

arvoelke · 2015-10-05T15:53:58Z

Yeah, I was mainly trying to motivate the logic for moving it into __init__. 😉

tbekolay · 2015-10-05T15:59:32Z

How would it work in __init__? A separate neuron_input and neuron_output boolean flag? That wouldn't work for my use case. I have an ensemble array that normally doesn't need neuron input or output. However, for debugging, it was convenient to add in a neuron output in order to look at the spike raster. The ensemble array was already created (in a function called from another function) so it would have been cumbersome to pass along a flag across a bunch of functions. Having a function that I could call after the network's been created allowed me to keep all that network creation code the same, and just call a function and add a probe when I needed to do that debugging.

arvoelke · 2015-10-05T16:10:14Z

Ah, I see... In that case I guess it's easier to make it consistent with add_output. It can technically be called multiple times already, so that's fine. The only part that's not technically consistent is the None return value, so just make it return self.neuron_input / self.neuron_output?

hunse · 2015-10-05T16:37:10Z

The thing I don't like about that is people might think they have to call it each time to get the neuron output, and not realize it's adding lots of nodes (though the warning should help).

jgosmann · 2015-10-05T16:52:17Z

The thing I don't like about that is people might think they have to call it each time to get the neuron output, and not realize it's adding lots of nodes (though the warning should help).

Wouldn't that be clear enough from the name saying add_... (and not get_...)?

hunse · 2015-10-05T17:09:41Z

I guess the way @tbekolay has it now, it won't actually add a second neuron input or output, it will just give the warning and return. So we could just have it return the existing one there, and it'd be fine. @jgosmann, you're right that add_ should make it clear, but what's not clear is how to access the thing that's been added. It might be good to add docstrings to the add functions saying where they put the added nodes.

tbekolay · 2015-10-05T19:34:19Z

Added a fixup commit to address comments.

arvoelke · 2015-10-05T19:42:21Z

nengo/networks/workingmemory.py

        nengo.Connection(net.gate, net.diff.neuron_input,
                         transform=np.ones((n_total_neurons, 1)) * -10,
                         synapse=None)

        # reset input (if reset=1, remove all values, and set to 0)
        net.reset = nengo.Node(size_in=1)
+        net.mem.add_neuron_input()
        nengo.Connection(net.reset, net.mem.neuron_input,


Nit: could demonstrate the alternative of moving the above line net.mem.add_neuron_input() to replace net.mem.neuron_input.

arvoelke · 2015-10-05T19:42:29Z

LGTM

hunse · 2015-10-05T19:47:03Z

LGTM too. When Travis passes I'll merge.

arvoelke · 2015-10-05T19:48:46Z

Seems like something might be wrong with Travis. Job 2014 was terminated after hanging for 10 minutes?

tbekolay · 2015-10-05T19:49:55Z

Looks like that job wasn't rebased to master, so it was still using a really slow NumPy.

tbekolay · 2015-10-05T19:50:52Z

Also, warning to @hunse, pushed another commit to address the nit, and fix a typo I noticed in ensemblearray's docstring.

hunse · 2015-10-05T19:52:03Z

Should this have a changelog?

tbekolay · 2015-10-05T19:54:18Z

Oh dag, you right. Writing one now!

tbekolay · 2015-10-05T19:57:24Z

Done.

Previously, these both happened whenever `neuron_nodes=True` was passed in. However, there are occasions when you only want a neuron input (as occurred in `InputGatedMemory`) or only want a neuron output (for plotting a raster, for example). Additionally, adding these functions makes the `__init__` function a bit more readable. For the time being, I've kept `neuron_nodes` around as a shortcut to calling `add_neuron_input` and `add_neuron_output`. However, I've added a DeprecationWarning when it's set to True; we should remember to remove `neuron_nodes` in Nengo 2.2. I also redid the unit tests appropriately, and moved the EA.dimensions property up in the file, as I tend to expect all properties to be organized before all methods.

tbekolay added the needs review label Oct 4, 2015

tbekolay added this to the 2.1.0 release milestone Oct 4, 2015

arvoelke reviewed Oct 5, 2015
View reviewed changes

tbekolay mentioned this pull request Oct 5, 2015

Things to change in 3.0 #822

Closed

13 tasks

arvoelke reviewed Oct 5, 2015
View reviewed changes

hunse force-pushed the refactor-ea branch from 80114be to 1ab607b Compare October 5, 2015 21:42

hunse merged commit 1ab607b into master Oct 5, 2015

hunse deleted the refactor-ea branch October 5, 2015 21:43

tbekolay added the reviewed label Aug 8, 2016

tbekolay removed the needs review label Aug 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EnsembleArray.add_neuron_input/output. #868

Add EnsembleArray.add_neuron_input/output. #868

tbekolay commented Oct 4, 2015

hunse commented Oct 4, 2015

tbekolay commented Oct 4, 2015

tcstewar commented Oct 4, 2015

arvoelke Oct 5, 2015

arvoelke commented Oct 5, 2015

hunse commented Oct 5, 2015

arvoelke commented Oct 5, 2015

tbekolay commented Oct 5, 2015

arvoelke commented Oct 5, 2015

hunse commented Oct 5, 2015

jgosmann commented Oct 5, 2015

hunse commented Oct 5, 2015

tbekolay commented Oct 5, 2015

arvoelke Oct 5, 2015

arvoelke commented Oct 5, 2015

hunse commented Oct 5, 2015

arvoelke commented Oct 5, 2015

tbekolay commented Oct 5, 2015

tbekolay commented Oct 5, 2015

hunse commented Oct 5, 2015

tbekolay commented Oct 5, 2015

tbekolay commented Oct 5, 2015

Add EnsembleArray.add_neuron_input/output. #868

Add EnsembleArray.add_neuron_input/output. #868

Conversation

tbekolay commented Oct 4, 2015

hunse commented Oct 4, 2015

tbekolay commented Oct 4, 2015

tcstewar commented Oct 4, 2015

arvoelke Oct 5, 2015

Choose a reason for hiding this comment

arvoelke commented Oct 5, 2015

hunse commented Oct 5, 2015

arvoelke commented Oct 5, 2015

tbekolay commented Oct 5, 2015

arvoelke commented Oct 5, 2015

hunse commented Oct 5, 2015

jgosmann commented Oct 5, 2015

hunse commented Oct 5, 2015

tbekolay commented Oct 5, 2015

arvoelke Oct 5, 2015

Choose a reason for hiding this comment

arvoelke commented Oct 5, 2015

hunse commented Oct 5, 2015

arvoelke commented Oct 5, 2015

tbekolay commented Oct 5, 2015

tbekolay commented Oct 5, 2015

hunse commented Oct 5, 2015

tbekolay commented Oct 5, 2015

tbekolay commented Oct 5, 2015