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

Fix keyword spotting example #103

Merged
merged 2 commits into from Oct 12, 2018

Conversation

3 participants
@tbekolay
Member

tbekolay commented Sep 29, 2018

@pblouw reported that his keyword spotting demo is broken in v0.3.0, and indeed our keyword spotting example is broken also. He tracked it down to this commit, which seems like it shouldn't even affect the keyword spotting network, but in fact it does.

The reason why that commit affects the keyword spotting network is that it translates all neuron->* connections in which pre is onchip and post is offchip with transform=weights to a solver=NoSolver(weight.T) connection. This would be fine if those two connections made no difference to how the network behaves, but that is unfortunately not the case, as evidenced by the keyword spotting example failing.

Investigating this issue is made more complicated by the fact that it is impossible to compare the two connection types because transform=weights connections are translated to NoSolver connections by the splitter. Commenting out this behavior in the splitter results in obvious differences between the two.

One way we can still investigate this is in looking at neuron->* connections in which pre is offchip and post is onchip, as this case was not handled in the same way as the reverse of that, so transform=weights connections still act differently from solver=NoSolver connections. This is definitely an oversight, but in this case a convenient one as it allowed me to write the test_n2n_transform_solver test in this PR.

Running that test with the commit it's introduced in (9042d86) reveals some hard to debug issues. Thanks to the new allclose changes, here's the first bunch of failures when comparing a connections with a transform and a NoSolver:

allclose first 5 failures:
  (22, 0): 0.005813399449355831 -0.027141482844726717
  (23, 0): -0.027332068748132363 -0.026265229725634918
  (24, 0): -0.026442792216304224 -0.025404153073492673
  (25, 0): 0.051607908894409936 -0.024571305871791547
  (26, 0): 0.04992955092214089 0.05341804061865481
  (27, 0): 0.04829266554173085 0.051680339469823776
  (28, 0): 0.04670944364715465 0.0170311741195067

I attempted to fix this in splitter in f2ae792. I was not able to fix it (I find the ChipReceiveNeurons and HostSendNode stuff quite confusing), but looking at how it fails now gives me some hope:

allclose first 5 failures:
  (11, 0): -0.037291051016988146 0.0
  (12, 0): -0.03607528021115636 0.0
  (13, 0): 0.0058673941456823045 0.0
  (14, 0): 0.005681813349207329 0.0
  (15, 0): 0.0054955413512869184 0.0
  (16, 0): 0.005315376075829346 0.0
  (17, 0): -0.014805258804706747 -0.037291051016988146

It looks like the NoSolver case might be operating the same as the transform case, only 6 timesteps delayed. We should make them the same, but seeing as I tried to do that here, I think I will need help to do that.

Also, even if we fix this case, it doesn't fix the keyword spotting example. However, it might give us a bit of insight into why the switch from the transform connection to the NoSolver connection made a difference. Does the way we did the connection splitting in 0811e2a have the same weird issue as in 9042d86, or is it simply delayed by some timesteps? My guess is that having several timesteps delay would not cause the keyword spotting example to change in the way that it changed, so I think we need to revisit the change in 0811e2a and ensure that switching the type of connection doesn't change behavior.

@tbekolay tbekolay force-pushed the fix-ks branch from 11c5be9 to f2ae792 Sep 29, 2018

@drasmuss drasmuss added this to In progress in PR status via automation Oct 10, 2018

@drasmuss drasmuss moved this from In progress to Ready for collaboration in PR status Oct 10, 2018

@tbekolay tbekolay added this to To do in 0.4 release Oct 11, 2018

@drasmuss drasmuss self-assigned this Oct 11, 2018

@drasmuss

This comment has been minimized.

Contributor

drasmuss commented Oct 11, 2018

See https://gitter.im/nengo/loihi-103 for some discussion points

@drasmuss drasmuss force-pushed the fix-ks branch from f2ae792 to cef7697 Oct 11, 2018

@drasmuss

This comment has been minimized.

Contributor

drasmuss commented Oct 11, 2018

Rebased to #112 and reverted #85. I pushed the old branch to https://github.com/nengo/nengo-loihi/tree/fix-ks-saved. @tbekolay wrote a test and did some work there related to chip/host communication, but not directly related to the keyword demo, which we might want in the future.

@drasmuss drasmuss removed their assignment Oct 11, 2018

@drasmuss drasmuss moved this from To do to Needs review in 0.4 release Oct 11, 2018

@tbekolay tbekolay force-pushed the fix-ks branch from cef7697 to 03b49c2 Oct 12, 2018

@tbekolay

This comment has been minimized.

Member

tbekolay commented Oct 12, 2018

This LGTM but since I made the original PR I can't submit a review. Will find a scapegoat then merge.

@tcstewar tcstewar self-requested a review Oct 12, 2018

PR status automation moved this from Ready for collaboration to Reviewer approved Oct 12, 2018

@tcstewar

This looks good to me as well. I will note that @pblouw seems to have fixed the keyword spotting demo when the connections are on-chip, so this PR might not be needed, but in the long run we're going to want a more robust way of controlling where the connection weights reside anyway.

drasmuss added some commits Oct 11, 2018

Revert "Move manual decoders onchip"
No longer transforms neuron connections into decoded connections.

Raises an explicit NotImplementedError for learning rules on
non-decoded connections.

@tbekolay tbekolay force-pushed the fix-ks branch from 03b49c2 to 23163ad Oct 12, 2018

PR status automation moved this from Reviewer approved to Done Oct 12, 2018

0.4 release automation moved this from Needs review to Done Oct 12, 2018

@tbekolay tbekolay merged commit 23163ad into master Oct 12, 2018

1 check passed

codecov/patch 90.47% of diff hit (target 77.48%)
Details

@tbekolay tbekolay deleted the fix-ks branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment