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

Check spike times for spike source array #61

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

chanokin
Copy link
Collaborator

@chanokin chanokin commented Jun 3, 2021

No description provided.

PyNN requires a test for spike times which are to be emitted by a SpikeSourceArray, they need to be provided 'in order' from lower to higher times.
@neworderofjamie
Copy link
Contributor

Why not just sort each array of spike times in SpikeSourceArray.get_extra_global_neuron_params?

@chanokin
Copy link
Collaborator Author

chanokin commented Jun 3, 2021

Why not just sort each array of spike times in SpikeSourceArray.get_extra_global_neuron_params?

The problem is that the test

@register()
def issue511(sim):
"""Giving SpikeSourceArray an array of non-ordered spike times should produce an InvalidParameterValueError error"""
sim.setup()
celltype = sim.SpikeSourceArray(spike_times=[[2.4, 4.8, 6.6, 9.4], [3.5, 6.8, 9.6, 8.3]])
assert_raises(InvalidParameterValueError, sim.Population, 2, celltype)

expects the population to throw an exception if the order is wrong

Copy link
Contributor

@neworderofjamie neworderofjamie left a comment

Choose a reason for hiding this comment

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

In that case, this makes sense and is a step towards implementing #19 more generally. Just a few suggestions

raise errors.InvalidParameterValueError(
"Spike times given to SpikeSourceArray must be in increasing order")
seq = seq.value
if len(seq):
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use np.all(np.diff(x) >= 0) then it handles the case of empty arrays

@@ -241,6 +241,8 @@ def build_genn_neuron(self, native_params, init_vals):
return self.build_genn_model(self.neuron_defs, native_params,
init_vals, creator)

def _test_parameters(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think _validate_parameters might be a better name - test implies something about testing (to me at least)

@@ -109,6 +109,8 @@ def __init__(self, size, cellclass, cellparams=None, structure=None,
sanitize_label(label))

def _create_cells(self):
self.celltype._test_parameters()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe this should be called after you've made the deep copy of the parameters and set it's shape (so you'd need something more like self.celltype._test_parameters(self._parameters)). Otherwise, I suspect spike_times.shape may often be None (I can't honestly remember when that happened but it definitely does)

@neworderofjamie
Copy link
Contributor

I was just checking on the build machine and, this change has now caused https://github.com/genn-team/pynn_genn/blob/master/test/system/scenarios/test_procedural_api.py#L9 to fail. I can't quite see why tbh...

@chanokin
Copy link
Collaborator Author

I think test_procedural_api.py: ticket195() was failing due to this. I had forgotten to remove an incorrect version of this test from the 'use_pynn_array_test_utils' branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants