Allow arbitrary learning rule size_in

[drasmuss]

Motivation and context:
This is the resolution to #1307, allowing learning rules to be defined with arbitrary size_in. Allows for more flexibility in the types of learning rules that can be implemented.

Also removes ConnectionParam, which wasn't actually being used anywhere.

I wasn't sure if we want a changelog entry for this (it's kind of user-facing, but only insofar as users are using the pluggable builder system).

Interactions with other PRs:
Based on discussion in #1307

How has this been tested?
This doesn't actually change anything in the current code base (since we don't have any rules with size_in that aren't covered by the previous error_type). But I added a test with a custom rule type
that has a non-standard size_in.

How long should this take to review?

• Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

• New feature (non-breaking change which adds functionality)

Checklist:

• I have read the CONTRIBUTING.rst document.
• I have updated the documentation accordingly.
• I have included a changelog entry.
• I have added tests to cover my changes.
• All new and existing tests passed.

[Seanny123]

LGTM. This change seems to have really improved the understandability of the code while adding more functionality.

[jgosmann]

I'd vote for adding a changelog entry because it is not possible to access the error_type attribute anymore on LearningRuleType objects and there is a new attribute size_in. These are both user-facing changes unrelated to the builder (even though most likely no one accessed these outside of the builder).

[hunse]

@jgosmann directed me here from #1311.

My problem with this is that the size is now specific to the LearningRuleType object. For example, say the error for my learning rule is always in the size of the pre object. With this PR, I have to make a new instance of my LearningRuleType for each connection, and set it's size_in to be the size of the pre object. In #1311, I can set my error_type to be "pre", and this handles that case.

I think the best solution is to combine this with #1311. The way I would do this is to make size_in also able to take particular strings, namely "pre", "post", and "decoded". "pre" would make size_in = pre.size_out, "post" would make size_in = post.size_in, and "decoded" would make size_in = post_obj.ensemble.size_in if isinstance(post_obj, Neurons) else post.size_in. (I would also be fine having None do the job of "decoded", as @drasmuss has done here, or having a different string instead of "decoded".)

If people are on board with that, I'm happy to implement it.

[jgosmann]

With this PR, I have to make a new instance of my LearningRuleType for each connection, and set it's size_in to be the size of the pre object.

I think in the general case it will be necessary to create LearningRuleType instances for each connectios with the appropriate dimensionality, for example with the the AML, when I combine both error signals, I always get pre size + post size, but have to set the size_in still manually. But this is just to say that we can't possibly automate all cases (at least not without larger changes). There are certainly common cases (like 'decoded' or 'post') that should not require setting size_in manually and 'pre' could very well be one of them. Thus, I'm on board.

[drasmuss]

I think the best solution is to combine this with #1311. The way I would do this is to make size_in also able to take particular strings, namely "pre", "post", and "decoded". "pre" would make size_in = pre.size_out, "post" would make size_in = post.size_in, and "decoded" would make size_in = post_obj.ensemble.size_in if isinstance(post_obj, Neurons) else post.size_in. (I would also be fine having None do the job of "decoded", as @drasmuss has done here, or having a different string instead of "decoded".)

Yeah that's what I was thinking. I'd probably drop None, just because when there's more than one "autocomplete" value it becomes more ambiguous which one None refers to.

[tbekolay]

I'd probably drop None, just because when there's more than one "autocomplete" value it becomes more ambiguous which one None refers to.

+1, combining with special strings sounds good, and I'd drop None as well.

[hunse]

Ok, added two commits. The first one does what I proposed.

The second allows expressions of these constants, so for example @jgosmann can do size_in='pre + post' for his learning rule. It doesn't add too much complexity, so I think it's worth it, but I'm fine just with the first commit if that's what people prefer.

[drasmuss]

I don't have a super rigorous argument for this, but I worry that the expressions is getting a bit fancy/fragile/evaly. Having strings that are one-to-one matches with some well-defined behaviour is fairly common, having strings that are evaluated as python expressions less so (outside of nengo.spa 😉). I'd be inclined to just have the user specify size_in manually in those cases. But that's just based on a fuzzy line I've drawn in my head.

[hunse]

I don't have a super rigorous argument for this, but I worry that the expressions is getting a bit fancy/fragile/evaly. Having strings that are just one-to-one matches with some well-defined behaviour is fairly common, having strings that are evaluated as python expressions less so (outside of nengo.spa ;) ). I'd be inclined to just have the user specify size_in manually in those cases. But that's just based on a fuzzy line I've drawn in my head.

The thing that concerns me is that errors will be harder to debug.

My reason for it is that there are two types of users: developer users (who will be writing new learning rules), and end users (who will be using them). Allowing expressions makes things slightly harder for developer users: They have to be more careful, and errors could be harder to debug. But it makes things easier for the end users, who can then take a learning rule and put it in their network without having to worry about getting the size in correct on each individual connection. To me, that's a worthwhile tradeoff.

Thinking about this now, though, to use something like @jgosmann's learning rule users will have to use the sizes of the pre and post objects explicitly anyway, since they'll need to slice into the proper parts of the error signal. So what we really need is learning rules that allow separate error signals, and allowing expressions for the size_in is just a hack that doesn't really fix the problem.

[hunse]

Ok, let's just go with the simple solution. How does that look?

[jgosmann]

Added a fixup commit because strings seemed to be too unspecific to me and as it is only used in one place the slightly longer name should not hurt.

[hunse]

Sure, looks good.

[tcstewar]

I don't have a super rigorous argument for this, but I worry that the expressions is getting a bit fancy/fragile/evaly. Having strings that are one-to-one matches with some well-defined behaviour is fairly common, having strings that are evaluated as python expressions less so (outside of nengo.spa 😉). I'd be inclined to just have the user specify size_in manually in those cases. But that's just based on a fuzzy line I've drawn in my head.

I share that concern. I'm pretty sure that in this case, I'd prefer having the user specify size_in, since in this case the "user" is someone fairly advanced (i.e. they're defining a new LearningRuleType). If I'm understanding things correctly, anyone who actually wanted to use that new LearningRuleType would not have to specify size_in. So I don't think it's worth adding the extra complexity of passing in expressions.

[tcstewar]

Never mind -- ignore my above comment. Looks like it got sorted out without me. :)

[drasmuss]

I pushed a small fixup that changes the new test to avoid slicing on multiple axes. That works OK in nengo, but causes problems in nengo_dl (and I think nengo_ocl as well).

[Seanny123]

I added a changelog entry, but one of the tests are still failing for reasons I don't understand. Any recommendations?

[jgosmann]

I got a similar error in my original PR #1310 which I fixed with commit 67aa36e. But it is a bit weird that this only happens for NumPy 1.7 ... maybe time to drop support for NumPy 1.7? But I guess we should first understand the exact reason for the failure.

[hunse]

Maybe we're calling array_equal on something, which then get cast to string arrays, but array_equal can't handle in older Numpy.

EDIT: I bet it happened when I added the strings to size_in. That would make sense.

[jgosmann]

I cherry picked the aforementioned commit, the Parameter code was based on the assumption that all parameter value are numeric or array-like (which is violated for strings and objects with custom __eq__ implementations). I'd recommend moving that commit to the beginning of the PR's commit history before merging to get every individual commit to pass the tests.

[Seanny123]

Thanks for figuring out that error @jgosmann and @hunse! This still LGTM.