Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple identical indexed slices are ignored #947

Closed
tcstewar opened this issue Feb 11, 2016 · 6 comments
Closed

Multiple identical indexed slices are ignored #947

tcstewar opened this issue Feb 11, 2016 · 6 comments
Labels

Comments

@tcstewar
Copy link
Contributor

Nengo allows you to make Connections by giving a list of dimension indices

Connection(a[[2,3]], b[[1,7]])

However, if you have the same index multiple times for the post part of the Connection, all but the last one are ignored:

Connection(a[[2,3]], b[[1,1]])

This ignores the mapping from dimension 2 to dimension 1, and just does the mapping from dimension 3 to dimension 1.

Here is a complete example:

import nengo
model = nengo.Network()
with model:
    stim = nengo.Node([-1, 0.5])
    a = nengo.Ensemble(n_neurons=100, dimensions=1)

    nengo.Connection(stim[[0, 1]], a[[0, 0]])

This model only sends the value 0.5 to the Ensemble a, and ignores the other dimension.

I believe the correct behaviour would be to send both dimensions to a (i.e. equivalent to a transform of [[1, 1]]), but another alternative would be to throw an error saying that indexed slicing doesn't support this case.

@tcstewar tcstewar added the bug label Feb 11, 2016
@hunse
Copy link
Collaborator

hunse commented Feb 11, 2016

Ooh, yeah, that's tricky. The problem is in SlicedCopy, because it's doing a copy, it will copy the first dimension, and then overwrite it with the second dimension. Maybe an error would be best for now, until we can think of a nice way to get this to work.

One option for implementing it might be to have SlicedCopy zero everything in the target buffer and then do an increment. It might be a minor performance hit, but acceptable I think. We could mitigate the performance hit by only doing this if one of the slices is a list of indices. (EDIT: or rather if the target is a list of indices)

@hunse
Copy link
Collaborator

hunse commented Sep 20, 2017

Looking at this again, this doesn't even work as you would think in Numpy. If you do

a = np.array([-1, 0.5])
b = np.array([0.])
b[[0, 0]] += a[[0, 1]]
print(b)

you get 0.5 (just the last value), not -0.5 as you would expect if both values had been added in. So just making the Copy increment the target value (as I suggested above) won't solve the problem.

This Numpy behaviour is going to make it difficult to do in Nengo without some special-case code that could really slow things down (because I think we'd have to loop over the indices). If anyone can figure out a fast Numpy expression to do this indexing operation, then post it here and we can go from there.

@tcstewar
Copy link
Contributor Author

The more I think about it, the more I'm tempted to just raise an Exception if someone does this.... I can't think of a situation where I'd really want to do this, and if numpy doesn't even produce the expected behaviour, then I feel justified in just not letting people do it..... :)

@hunse
Copy link
Collaborator

hunse commented Sep 20, 2017

It looks like newer Numpy has a helper function at on basic operators just for this purpose: https://stackoverflow.com/questions/24099404/numpy-array-iadd-and-repeated-indices

The downside of using that is then we'd have different behaviour on Numpy < 1.8 and Numpy >= 1.8, or else we'd just have to stop supporting < 1.8.

@tbekolay
Copy link
Member

Hm... the past two stable releases of Debian (since April 26, 2015) have had at least Numpy 1.8, so I'm OK bumping up the dependency. Any objections?

@hunse
Copy link
Collaborator

hunse commented Sep 20, 2017

The earliest current Ubuntu release (trusty 14.04), also has 1.8, so I'm good to bump it.

tbekolay pushed a commit that referenced this issue Sep 25, 2017
Also test that boolean indexing works. With this, advanced indexing
is fully supported for ObjViews in connections.

Fixes #947.
tbekolay pushed a commit that referenced this issue Sep 25, 2017
Also test that boolean indexing works. With this, advanced indexing
is fully supported for ObjViews in connections.

Fixes #947.
tbekolay pushed a commit that referenced this issue Sep 25, 2017
Also test that boolean indexing works. With this, advanced indexing
is fully supported for ObjViews in connections.

Fixes #947.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants