Combined decoders and transform

[hunse]

I combined decoders and transform in the connection builder. This makes things a lot cleaner, and it should be easier for users too, since they don't have to decide between probing 'transform' or 'decoders' when doing learning (right now it depends on the type of connection).

I also improved slicing, so we support slicing on neuron-to-neuron connections.

[hunse]

Okay, this is ready for review!

[tbekolay]

I also improved slicing, so we support slicing on neuron-to-neuron connections.

Oh awesome, I ran into that bug recently :)

[arvoelke]

Do we have any situations where we learn decoders on PES and then apply a transform? Would that still work? Say if I'm learning a scalar and then projecting that into a higher dimensional space. Or do we now need to apply the same transform to the error signal in this case?

[hunse]

Do we have any situations where we learn decoders on PES and then apply a transform? Would that still work? Say if I'm learning a scalar and then projecting that into a higher dimensional space. Or do we now need to apply the same transform to the error signal in this case?

I think you would need to apply the same transform on the error signal, or at least account for it in some way.

[hunse]

Okay, this is rebased and ready for review.

The removing 'decoders' from BuiltConnection is optional; we can discuss if we want to do this or not.

[arvoelke]

I had been using PES assuming that the error is w.r.t. the decoded vector of dimension size_mid, not the transformed vector of dimension size_out. That's how it worked before, but we had no unit tests to assert this behaviour (so I guess I had been taking advantage of undefined behaviour). The old behaviour seemed natural to me (e.g. projecting a learned scalar to each dimension in a product network), but they are equivalent.

I've added a unit test to assert the new behaviour here (and fixed a line to make it pass): 91d59a3

Note: if you change line 173 to have no transform, then that's what would have passed on master. Feel free to cherry-pick this in if we're in agreement.

I'll take a look at the rest soon.

[arvoelke]

• Rename to sliced_signal to be consistent with filtered_signal, and since get usually implies no side-effects
• Docstring
• Change assertion to exception

[arvoelke]

Gone over it pretty carefully and it looks good and well thought out. I'm fine with the renaming to weights, but regardless this all seems important enough to warrant the all-important changelog entry (especially since this altered a bunch of our examples, and therefore our documentation).

[hunse]

One thing to note is that with this PR, we no longer use full_transform. Should we remove it? This would affect #754 and #755.

[hunse]

Addressed @arvoelke's suggestions. I kept the asserts in, because I think they should only ever be seen if we mess something up (i.e., users should never see them).

I also added a commit to remove the as_update argument from the Copy and DotInc operators, since it was unused in both cases.

[arvoelke]

Cherry-pick 91d59a3 and update the change log? Then it looks good to me.

[arvoelke]

LGTM. Would be good for someone else to look this over now and get it in soon as it's a pretty big change.

[tbekolay]

I'll give this a review tonight or early tomorrow!

[tbekolay]

OK, reviewed! This looks great to me. Only a few issues, some of which I can do during the merge.

1. As commented inline, are you sure we need to remove decoders from BuiltConnection? I know Chris used this for a learning notebook recently...
2. Is it still possible to probe 'decoders' and 'transform'? If so, then I think we should allow this, but raise a deprecation warning and remove that ability in 2.2. If not, then perhaps we should allow this for now with the deprecation warning? In either case, we need a stronger changelog message, as it means end users will need to change their models (I can write this in the merge though).

I'll aim to merge this in today.

[hunse]

My rationale for removing "decoders" from BuiltConnection was that it would be pretty rare for someone to want the decoders without the transform in it. I've started thinking about the "transform" and "function" on connections as going hand in hand (in fact, there's an argument that we shouldn't really even have "transform" on an ensemble->ensemble connection, because you can always build it in to "function", but let's not go there now since it makes problems for other connection types). Anyway, if you want the decoders for the "function" that your connection is computing, I think it makes the most sense if that includes the transform as well, since the transform is also part of the "function" you're computing.

As for probing "decoders" and "transform", it's not possible to probe them in the way you could before, which is why I chose "weights" as the name for the new matrix. That way, whether we're talking about BuiltConnection or probing, it's clear that "weights" is different from the old "transform" or "decoders". It would be good to have some error notifying users that "decoders" and "transform" are obsolete, if users try to probe them, but I can't think of any sensible way to map them to what's there now (mapping both "transform" and "decoders" to "weights" would cause problems, I think, because probes would appear to continue working but not return what they used to).

[tbekolay]

Makes sense @hunse! I was working on adding a more helpful error message for when you try to probe decoders / transform, but it kind of became its own beast (see #781) so I'm happy to merge this as is. I'll give this a final lookover and history fixup tomorrow morning and then merge.

[tbekolay]

Fixed up history. Squashed the 'removing decoders from BuiltConnection' commit into the main one, as without it most models won't build. Merging in a few minutes unless there are last minute objections!