Skip to content
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

Seeds match Nengo #70

Merged
merged 1 commit into from Feb 21, 2019

Conversation

Projects
None yet
2 participants
@hunse
Copy link
Contributor

hunse commented Sep 18, 2018

This ensures that all object seeds are the same as for the corresponding reference Nengo model. It also ensures that seeds for all objects are set and the same whether precompute is True or False.

Depends on nengo/nengo#1476.

Computing the seeds before building, as I'm doing here, is something we've talked about adding to Nengo for a while. I would be open/happy to take the functionality here and move it to Nengo instead, since I think that is ultimately where we want it.

@hunse hunse referenced this pull request Sep 18, 2018

Closed

Do not overwrite existing seeds when building #1476

0 of 6 tasks complete
@hunse

This comment has been minimized.

Copy link
Contributor Author

hunse commented Sep 18, 2018

This branch is off of #71, but the functionality doesn't depend on it. It was just in trying to get that stuff working that I stumbled upon this problem, so I was using it as a test case. So just look at the last commit when reviewing this.

@drasmuss

This comment has been minimized.

Copy link
Contributor

drasmuss commented Dec 4, 2018

Rebased this to master

@hunse

This comment has been minimized.

Copy link
Contributor Author

hunse commented Feb 1, 2019

Rebased this to #142, so that it can work with current Nengo.

@hunse hunse referenced this pull request Feb 1, 2019

Merged

Seed backends #1501

4 of 5 tasks complete
@hunse

This comment has been minimized.

Copy link
Contributor Author

hunse commented Feb 1, 2019

This requires nengo/nengo#1501 to work.

@drasmuss drasmuss force-pushed the seeds branch from a199610 to ae8b64d Feb 8, 2019

@drasmuss
Copy link
Contributor

drasmuss left a comment

Added some minor fixups, looks good to me

@@ -208,7 +208,7 @@ def test_multiple_pes(init_function, allclose, plt, seed, Simulator):

probe = nengo.Probe(output, synapse=0.1)

simtime = 0.5
simtime = 0.6

This comment has been minimized.

@hunse

hunse Feb 8, 2019

Author Contributor

Why?

This comment has been minimized.

@drasmuss

drasmuss Feb 8, 2019

Contributor

This test was failing, but I'm pretty sure it was just because the seed changed (running it various times with different seeds it would sometimes randomly fail). Increasing the time made it pass reliably.

This comment has been minimized.

@hunse

hunse Feb 11, 2019

Author Contributor

If switching to 0.6 makes it work for a wider range of seeds, probably worth the change, then. Maybe we can add a short statement in the commit message saying this?

As I mentioned below, the randomness of this test is also fixed in #140, so between the two changes, should be solid!

@hunse

This comment has been minimized.

Copy link
Contributor Author

hunse commented Feb 8, 2019

First fixup looks good to me. I added some comments just to make it clear that there's two sections to the test.

I'm curious why the second fixup is necessary. If it's because that test was failing, it just fails randomly. It's fixed in #140.

All split networks use same seeds as Nengo
This is true for the host, host_pre, and chip networks.

@drasmuss drasmuss force-pushed the seeds branch from f5c9132 to 9cef210 Feb 21, 2019

@drasmuss drasmuss merged commit 9cef210 into master Feb 21, 2019

@drasmuss drasmuss deleted the seeds branch Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.