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

Added before_run codeblock in numpy and C++ templates #1603

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

Conversation

mahipalimkar
Copy link

Moved the Python code in SpikeGeneratorGroup.before_run into the before_code blocks of the numpy and C++ templates.

@mstimberg
Copy link
Member

Apologies, I did not yet have time to have a look at the PR, but I will do so tomorrow !

Copy link
Member

@mstimberg mstimberg left a comment

Choose a reason for hiding this comment

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

This is a good start! I only looked at the Python code so far, and there are a number of issues that prevent the code from running – I cannot yet judge whether it will actually work after fixing these issues, but let's take it one step at a time 😊

@@ -1,25 +1,48 @@
{# USES_VARIABLES { _spikespace, neuron_index, _timebins, _period_bins, _lastindex, t_in_timesteps } #}
{% extends 'common_group.py_' %}
{% extends 'common_group.py.jinja2' %}
Copy link
Member

Choose a reason for hiding this comment

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

It might have been better to call templates like this, but it is currently called common_group.py_.


{% block before_code %}
# Copy of the SpikeGeneratorGroup.before_run code
dt = {{dt.item()}} # Always copy dt
Copy link
Member

Choose a reason for hiding this comment

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

These variables are not available to the template, since it does not know that they are needed. You can use USES_VARIABLES to specify them. See https://brian2.readthedocs.io/en/stable/developer/codegen.html#template-keywords and other templates.

# Always recalculate timesteps
from brian2 import defaultclock # Use Brian 2's clock instead of 't'
current_t = defaultclock.t
timesteps = ({{_spike_time}} / dt).astype(np.int32)
Copy link
Member

Choose a reason for hiding this comment

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

The names used here have to be the names as added with self.variables.add_, not the name of the SpikeGeneratorGroup attribute – in this case, the name should be spike_time not _spike_time.

{% block before_code %}
# Copy of the SpikeGeneratorGroup.before_run code
dt = {{dt.item()}} # Always copy dt
period = {{period_}} # Always copy period
Copy link
Member

Choose a reason for hiding this comment

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

The _ suffix syntax is only used in Python user code to get the values of a variable without its units. In a template, variables are always the underlying numpy arrays without unit information, you can therefore simply use period here.

Suggested change
period = {{period_}} # Always copy period
period = {{period}} # Always copy period


{% block maincode %}
_the_period = {{_period_bins}}
_timebin = {{t_in_timesteps}}
_timebin = int(defaultclock.t / {{dt.item()}}) # Use Brian 2's clock instead of 't_in_timesteps'
Copy link
Member

Choose a reason for hiding this comment

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

See comment about .item() above.


# Always recalculate timesteps
from brian2 import defaultclock # Use Brian 2's clock instead of 't'
current_t = defaultclock.t
Copy link
Member

Choose a reason for hiding this comment

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

It should use {{t}} instead of the defaultclock.t (even though most of the time the two will be the same).

current_step = int(current_t / dt)

# Always update _lastindex
in_the_past = np.nonzero(timesteps < current_step)[0]
Copy link
Member

Choose a reason for hiding this comment

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

In the Python templates, numpy is available as _numpy, not np.

Suggested change
in_the_past = np.nonzero(timesteps < current_step)[0]
in_the_past = _numpy.nonzero(timesteps < current_step)[0]


# Always recalculate _timebins
shift = 1e-3 * dt
timebins = np.asarray(({{_spike_time}} + shift) / dt, dtype=np.int32)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
timebins = np.asarray(({{_spike_time}} + shift) / dt, dtype=np.int32)
timebins = _numpy.asarray(({{_spike_time}} + shift) / dt, dtype=np.int32)

# Always update _lastindex
in_the_past = np.nonzero(timesteps < current_step)[0]
if len(in_the_past):
{{_lastindex}}[0] = in_the_past[-1] + 1
Copy link
Member

Choose a reason for hiding this comment

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

For scalar variables, Brian already applies the [0] automatically.

Suggested change
{{_lastindex}}[0] = in_the_past[-1] + 1
{{_lastindex}} = in_the_past[-1] + 1

@mahipalimkar
Copy link
Author

Thank you so much @mstimberg ! I can see I made a lot of errors 😓 I will make these changes at the earliest

@mstimberg
Copy link
Member

Working with this mix of Python code, Jinja templates, Brian's idiosyncratic variable system, etc. is really far from straightforward – I wouldn't expect anyone to get this right on the first try! For testing whether things work, try running any example that uses SpikeGeneratorGroup and add prefs.codegen.target = 'numpy' so that it uses the Python target instead of the default Cython.

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