Improved probing of Connection transform and decoders #671
Conversation
| sig = sig[probe.slice] | ||
| else: |
There was a problem hiding this comment.
There was no way this else was ever being reached, and as far as I can tell indexing slices works fine, so I think this was just left in by accident?
There was a problem hiding this comment.
Does this work with list indexing?
There was a problem hiding this comment.
As far as I could tell it did, unless there is some weird indexing method I was missing.
There was a problem hiding this comment.
So from what I can tell it doesn't work with list indexing:
a = nengo.Ensemble(100, 1)
ap = nengo.Probe(a.neurons[[0, 1, 5]], 'voltage')There was a problem hiding this comment.
Seems to be something weird with neurons
a = nengo.Ensemble(10, 3)
p = nengo.Probe(a[[0,2]])works fine.
There was a problem hiding this comment.
That's because probing an ensemble makes a "connection_probe", as will probing neurons. Since this uses the Connection builder code, which supports advanced slicing, it works fine. It's only when you make a "synapse_probe" (the function in which the line in question is located), for example probing the voltages on neurons, that you run into problems with advanced slicing.
There was a problem hiding this comment.
Ah alright, we can put the warning back in then. Or I guess we could implement it, but that seems like more work 😓
There was a problem hiding this comment.
I would put the exception back in for now.
|
Agreed; |
Similar to previous commit doing this for `Connection`.
It was previously only used for `probeable` attributes, so now there's no need for them. If other uses come up, we can revive it from the history.
|
looks good! |
Fix for #665 and #667
The only substantive change this makes is that
probeableis no longer aParameter. And I think the only real difference that makes is that you can't use the config system to adjust it. But I think it was always a bit weird to haveprobeablebe a parameter like that. It's really an internal variable, not a model parameter, and I don't think there's any good reason why the user would want to configure it. I would actually be in favour of changing all the otherprobeablelists to be the same, but I didn't put that in here.