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

Test probes #169

Merged
merged 4 commits into from Jan 31, 2019
Merged

Test probes #169

merged 4 commits into from Jan 31, 2019

Conversation

hunse
Copy link
Collaborator

@hunse hunse commented Jan 17, 2019

This adds a test for neuron input (current) probes, which were not working correctly (due to negative values not being cast correctly, and integers not being filtered correctly). This also corrects both these issues.

TODO:

  • Add a test for voltage probes as well. The voltage should go negative, to test that we're dealing with negatives correctly. Do we currently allow min_voltage < 0 for e.g. LIF neurons?

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! I have a few questions in inline comments and we should add a changelog entry, as this fixes bugs and supports lif.min_voltage. This may also allow some Nengo tests to pass, though I don't see any xpasses in TravisCI. It could be a simple tolerance issue though since Loihi min voltages are rounded, in which case we would have to wait for pytest-allclose to allow changing tolerances, so probably let's not worry about the Nengo tests for now.

The part of the coverage diff that isn't covered is on the chip (loihi_interface.py stuff), so another question is whether to wait for the CI running on hardware stuff. My vote would be no, but this also will be easy to rebase later on so either way 🤷

@@ -800,7 +800,7 @@ def _filter_probe(self, cx_probe, data):
shape = data[0].shape
synapse = cx_probe.synapse
rng = None
step = (synapse.make_step(shape, shape, dt, rng, dtype=data.dtype)
step = (synapse.make_step(shape, shape, dt, rng, dtype=np.float32)
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for float32 over float64 here? My default would be float64 since that's what float and np.float_ are.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to use less memory when we have a lot of probe data. Since the underlying values are all limited precision (since they come from Loihi), I figured that not much would be lost by using float32 instead of float64.

@@ -822,6 +822,7 @@ def get_probe_output(self, cx_probe):
if cx_probe.use_snip:
data = self._snip_probe_data[cx_probe]
if cx_probe.synapse is not None:
data = np.asarray(data, dtype=np.float32)
Copy link
Member

Choose a reason for hiding this comment

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

Ditto float32 vs 64 here

@@ -198,11 +198,12 @@ def _configure_filter(self, tau_s, dt):
"Current (U) scaling is required. Perhaps a synapse time "
"constant is too large in your model.")

def configure_lif(self, tau_rc=0.02, tau_ref=0.001, vth=1, dt=0.001):
def configure_lif(self, tau_rc=0.02, tau_ref=0.001, vth=1, dt=0.001,
min_voltage=0):
Copy link
Member

Choose a reason for hiding this comment

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

Since we're touching this, can we add a docstring and document these parameters?

@hunse
Copy link
Collaborator Author

hunse commented Jan 23, 2019

Addressed the requested changes. I've also added changelogs for both the probe fixes and the min_voltage.

@hunse hunse force-pushed the test-probes branch 2 times, most recently from c02f7e7 to 1728e5b Compare January 29, 2019 16:02
hunse and others added 2 commits January 31, 2019 13:13
In the snip, the data is int32. When being read by Python, it needs
to be explicitly interpreted as int32, otherwise it treats it as
int64 and negative numbers become large positive numbers.

This should fix probing of negative neuron inputs and voltages.
@drasmuss drasmuss force-pushed the test-probes branch 2 times, most recently from 523793e to 8888669 Compare January 31, 2019 17:43
@hunse
Copy link
Collaborator Author

hunse commented Jan 31, 2019

Fixups look good to me.

hunse and others added 2 commits January 31, 2019 15:29
Document CxGroup.configure* functions
These probes were untested and not working correctly on chip

test_voltage_decode no longer hangs
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

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