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

Ensure things happen in consistent order #151

Merged
merged 1 commit into from Jan 31, 2019
Merged

Conversation

hunse
Copy link
Collaborator

@hunse hunse commented Dec 7, 2018

  • Splitter preserves object order from original network.
  • Host/chip receivers are always called in same order.

This makes sure that seeds are always set the same as well. Right now, setting a seed on a network does not actually guarantee it will be the same from one run to the next, because the splitter dictionaries change the order that everything gets built in in a non-deterministic way.

Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Made some inline comments, and this needs a changelog.

It would also be nice if there was a test, but I have to admit I am struggling to think of a test that isn't trivial ... do you think this will end up getting tested in #70?

@@ -1164,12 +1166,12 @@ def receive(self, t, x):
class ChipReceiveNode(nengo.Node):
"""For receiving host->chip messages"""

def __init__(self, dimensions, size_out):
def __init__(self, dimensions, size_out, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think **kwargs is confusing here; for me, I don't really remember what other parameters there are to Node. If I look it up and see size_in, I'll get a TypeError if I try to specify it. I think we should bite the bullet and expose label and seed (or just label if seeds are irrelevant here).

@@ -1190,6 +1192,7 @@ def collect_spikes(self):

class ChipReceiveNeurons(ChipReceiveNode):
"""Passes spikes directly (no on-off neuron encoding)"""
def __init__(self, dimensions, neuron_type=None):
def __init__(self, dimensions, neuron_type=None, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also, explicitly include label and (perhaps) seed.

@hunse
Copy link
Collaborator Author

hunse commented Jan 23, 2019

I also can't think of a test that is non-trivial. Maybe #70 will test some of this, though also the point of #70 was to make sure that things have the same seeds as Nengo no matter what order they're built in.

I've addressed the comments and added a changelog.

@hunse hunse mentioned this pull request Jan 25, 2019
@tbekolay
Copy link
Member

From the review: we should add a test that splits a model multiple times and ensures that their order in the split networks is the same.

@hunse
Copy link
Collaborator Author

hunse commented Jan 29, 2019

I will write this test.

@hunse hunse self-assigned this Jan 29, 2019
@hunse hunse force-pushed the consistent-order branch 2 times, most recently from c7e42b6 to d83169c Compare January 29, 2019 01:08
@hunse
Copy link
Collaborator Author

hunse commented Jan 29, 2019

Ok, I've added a test. I've tested that it can distinguish between before this PR and after it.

@hunse hunse removed their assignment Jan 29, 2019
- Splitter preserves object order from original network.
- Host/chip receivers are always called in same order.

Also added optional labels to ChipReceiveNodes and their
CxSpikeInputs.
Copy link
Member

@drasmuss drasmuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just waiting for tests to run then will merge.

@drasmuss drasmuss dismissed tbekolay’s stale review January 31, 2019 14:59

Requested changes implemented

@drasmuss drasmuss merged commit cd3ba5a into master Jan 31, 2019
@drasmuss drasmuss deleted the consistent-order branch January 31, 2019 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants