Learning unmodulated #642
Learning unmodulated #642
Conversation
hunse
force-pushed
the
learning-unmodulate
branch
from
17a8729
to
c1ae217
Compare
|
I keep going back and forth on the error sign switch. It makes for a pretty sharp discontinuity with previous models. Also, the PES rule is essentially just a renaming of the delta rule, and the delta rule is usually written the way the PES rule is now (i.e. But you're right that in that case, referring to the feedback signal as an "error" is kind of odd. What we probably should have done is named it "feedback" instead of "error" or something. I don't know whether it's better to change our nomenclature to match the implementation, or change the implementation to match the nomenclature. |
|
Every rule is essentially just a renaming of the delta rule ;) Daniel Rasmussen wrote:
|
|
In that it's all just gradient descent, yeah, although some are more similar than others 😉. And in general with gradient descent the convention is to use the error signal as in this PR, which is the main argument for the swap I think. It's probably more likely that people used to things like backprop would be confused by the swapped error in the current implementation, as @hunse suggests. |
hunse
force-pushed
the
learning-unmodulate
branch
from
c1ae217
to
9169d9f
Compare
hunse
force-pushed
the
learning-unmodulate
branch
2 times, most recently
from
b31d72a
to
9822751
Compare
|
Do people have comments on this, or is it ready to go? |
|
I'll review this shortly. I'm considering the error signal sign flip uncontested! |
tbekolay
force-pushed
the
learning-unmodulate
branch
from
9822751
to
327245f
Compare
| @@ -83,10 +83,6 @@ def validate_ops(sets, ups, incs): | |||
| for node in sets: | |||
| assert len(sets[node]) == 1, (node, sets[node]) | |||
|
|
|||
| # -- assert that only one op updates any particular view | |||
| for node in ups: | |||
| assert len(ups[node]) == 1, (node, ups[node]) | |||
There was a problem hiding this comment.
Hm, is this OK? Why did we have this restriction in the first place?
There was a problem hiding this comment.
I couldn't think of a good reason to have it. The original reason may have just been because we always used updates for filters, in which case the signal in the filter being updated should only ever be updated once.
When you get into multiple updates per step, there is a possibility that the order of updates matters. I was going to say that that's not a problem if the updates are just increments, but that's not true. Any learning rule that depends on the magnitude of the connection weights will act differently if it's computed before or after another rule modifying the same weights. Maybe this is something we need to consider? We should at least be able to make this deterministic within a model, so I don't think it's too big of an issue.
There was a problem hiding this comment.
Yeah, I think this is something we need to consider... I'm sure the difference (in this case) is pretty small, but it could be large in some cases. I'm not sure how we'd order ops that both update the same signal... So yeah, I'd be keen to keep this assertion in and figure out another way to do these updates. I think the PreserveValue op from before was pretty good.
|
This was a bit old, so I gave it a rebase. Looking good! Only two things I think before merge. First is the all-important changelog entry. I also had a thought about the |
|
Adding that error sounds like a good idea. |
|
Updated the changelog and made it so that you get This introduced a new |
|
Should #643 be implemented here too, in order to contain these rather significant changes to PES to a single PR? (Note: The work is done, I just need to know where to attach) |
|
Hello Nengoers, I'm trying to restore my SpiNNaker version of PES to it's former glory and the learning rate seems to have been scaled somehow. In the old learn_communications_channel example it was 1 and now it's 1e-6 - How has the calculation of the PES update changed? THANKYOU! |
|
The implementation I was working on at last years summer school multiplied the learning rate by dt (in seconds) - Is that the scaling in question or something more cunning involved? |
|
It's a little bit more complicated, in that what's actually happening is that the output activities are being scaled by |
|
@neworderofjamie That sounds right, although the correct way to do it may depend on whether your spikes are scaled by where This PR just needs a final review now and then it should be good to go. |
|
Can we change Also, did we want to add scaling by number of pre neurons in this PR? Might as well change it now, and revise the learning rates again. EDIT: oh sorry, just saw the commit for that. |
| - ``LearningRuleType`` objects no longer take modulatory connections. | ||
| Instead, a connection should be made directly from the error population | ||
| to the learning rule. Also, the error's sign has been flipped for | ||
| the PES learning rule to be ``actual - target``. |
There was a problem hiding this comment.
"learning rule" -> "learning rule error"
|
Sorry about throwing more onto the pile, but realized that the pre-activity should be filtered. It was just a one line change in the builder, and a new parameter for PES. Note that the parameter order for PES is different from BCM, otherwise all the examples (and possible models that people have already built), need to change their learning rate to a key-word argument. Addressed @hunse comments. We agree'd that |
|
This makes sense to me; we don't use any neural info from the post population in decoder PES, so why not. I wonder if we should revisit the |
|
+1 to revisiting |
tbekolay
force-pushed
the
learning-unmodulate
branch
from
dceab9d
to
44768d4
Compare
|
OK, |
|
Added a fixup for neuron to ensemble learning connections. This should all be cleaned up in the |
|
Added a test for neuron to ensemble PES learning. I think this should be good to go, @tbekolay. |
|
Great; I'll look this over this afternoon! |
|
The new tests LGTM! @arvoelke mentioned that he wanted to give this a look over too. I'll probably merge tomorrow afternoon unless someone forcibly stops me. |
Connect the error directly into a learning rule (as discussed in #632), instead of making a modulatory connection. This is cleaner and clearer. Learning rules are now built by their parent connections, and since a connection to a learning rule must added to a network after the connection containing the learning rule, the learning rule will always be built when the builder for the error connection into said learning rule is called. This fixes #632 (slicing `post` in learned connections). The test `test_learning_rules.py:test_pes_decoders_multidimensional` has been updated to test this feature.
The PES rule previously moved in the direction of what it called the "error" signal, meaning that the error signal had to be the target value for the learning minus the actual value. This is the opposite of what people typically refer to as error. This commit changes the sign of this error so that it is actually an error, and the PES rule moves in the opposite direction of the error.
This operator is a bit hacky, but after some experimenting, this is essentially the cleanest way to ensure that all learning operations are applied correctly. Increments and reads happen *after* a signal is set. By setting the transform with PreserveValue, we allow increments to be done by learning rules. The only other alternative is to update it, which happens *last*. It's not obvious why one learning rule would be applied before another, so by using PreserveValue, we do the delta computations on the previous timestep, and then increment the transform on the current timestep's 'increments' phase. The transform isn't updated, but that's okay because it's set.
BCM and Oja calculate their delta, then apply it to the transform. PES skipped this step and just incremented the transform (or decoders) directly. By refactoring PES to calculate the delta and then apply it, we get two big benefits: 1. The order in which learning rules are applied no longer matters; the delta calculations are all done before the transform changes, since delta calcs are updates and the transform changes are incs. 2. The code for defining a delta signal and applying it to the transform can be refactored into the `LearningRule` builder, rather than being duplicated in each `LearningRuleType` builder. This should make learning rules easier to implement in the future. As another minor benefit, it's now possible to probe the 'delta' of each learning rule. This makes it easy to see exactly what each each learning rule is doing, which is especially useful when you're using multiple learning rules at the same time. Also made a few other minor enhancements: better docstrings for `SimOja` and `SimBCM`, and a note about ordering of connections in the `Network` builder.
tbekolay
force-pushed
the
learning-unmodulate
branch
from
075eb45
to
ed8a566
Compare
Previously, we used the type of the pre or post object as a clue to what the learning rule modifies. This makes it more explicit by saying what signal the learning rule actually modifies. This will make it easier to implement learning rules that modify other quantities, like for example `post.encoders`.
All PES tests now run through a single helper function, making the code more concise. I also added a test for neuron to ensemble PES learning, as well as a test for PES learning when using a weight solver.
|
OK, this branch is now rebased to master, and the history's been squashed and whatnot. I think this is ready to merge; will do so in an hour or so unless anyone stops me! |
|
Awesome! |
This makes it so that we connect directly to LearningRules instead of making modulatory connections, as discussed in #632. It fixes that problem.
The second commit switches the sign of the PES error, so that it is actually an error (i.e. the actual value minus the target value). This means that the PES learning rule moves in the opposite direction of the error, as one would expect (e.g., if the output is too large, make it smaller). This makes a lot more sense to me, and hopefully to everyone else as well. Now seemed like a good time to do it, because the changes from the first commit will require that everyone re-write their learning code anyway.
EDIT: this also addresses #366.