Add C++ DynamicArrays in Brian2 for runtime mode
#1650
Conversation
… a single contiguous std::vector<T> to enable zero-copy interop for numpy
|
@mstimberg Finally got it working! 🎉
|
|
As I mentioned in a comment, I will only have a closer look at the code later, but I just made a small commit to fix a test suite issue (it was using Brian directly from source to get the location of the Cython cache dir, but importing Brian from source now fails). If the tests still fail, it will be about actual problems in this PR, I hope 😉 |
…r older mode so some new features are not available
|
All tests pass now :) |
…ready takes a copy
|
@mstimberg so some more context here on that unusual problem we are facing with the 
so this is as far as we can go in terms of removing things that were changed in the test ... I'll revert back the above commit and also try to make sense of this more , like why the test fails if we remove this piece of code ( and that too as we know from earlier trials only when we run the full testsuite on all OS , if we run a partial testsuite just using the failing ubuntu OS that passes ) Ahh and also please whenever you get the chance , you can review the PR now as I guess we have hit the limit as to what changes we can remove from the test 😅 |
…as it already takes a copy" This reverts commit 732cbc5.
There was a problem hiding this comment.
I finally found some time to look into the PR in more details… Looks very good, I don't have any major objections, my main comments are about, well, comments, and some about potential to simplify the code. There's one big thing that is still missing: the statemonitor.pyx templates still goes through Python to resize the dynamic variable. This would make sense to change as part of this PR, I think. There is a similar issue for the synapse creation templates still calling owner.resize, but this is slightly more work to change, I'd be happy with doing this in a separate PR as well.
| for value, n_ambiguous in tests: | ||
| with catch_logs() as l: | ||
| G.v.__setitem__(SG, value) | ||
| assert ( | ||
| len(l) == n_warnings | ||
| ), f"expected {int(n_warnings)}, got {len(l)} warnings" | ||
| assert all( | ||
| [entry[1].endswith("ambiguous_string_expression") for entry in l] | ||
| ambiguous_found = sum( | ||
| [1 for entry in l if entry[1].endswith("ambiguous_string_expression")] | ||
| ) | ||
| assert ambiguous_found == n_ambiguous, ( | ||
| f"Expected {n_ambiguous} ambiguous warnings for value '{value}', " | ||
| f"but got {ambiguous_found}" |
There was a problem hiding this comment.
At least locally, this test seems to pass as it was before (i.e. without filtering by type of warning) – is there a reason to change it?
There was a problem hiding this comment.
ahh ...so the testsuite was failing earlier and I was not sure why , and since the test seemed trivial ( in the sense that warnings can change provided we changed alot of code ) so I redid the test a bit , let me try the older one again and if that works we can revert back to it :)
There was a problem hiding this comment.
@mstimberg I reverted the changes and now the test fails for me unfortunately...
so what should we do ?
There was a problem hiding this comment.
______________________________ test_state_variables_group_as_index_problematic ______________________________
@pytest.mark.codegen_independent
def test_state_variables_group_as_index_problematic():
G = NeuronGroup(10, "v : 1")
SG = G[4:9]
G.v = 1
tests = [("i", 1), ("N", 1), ("N + i", 2), ("v", 0)]
for value, n_warnings in tests:
with catch_logs() as l:
G.v.__setitem__(SG, value)
> assert (
len(l) == n_warnings
), f"expected {int(n_warnings)}, got {len(l)} warnings"
E AssertionError: expected 1, got 2 warnings
E assert 2 == 1
E + where 2 = len([('WARNING', 'brian2.core.variables.ambiguous_string_expression', "The string expression used for setting 'v' refers t.../python3.13/site-packages/_pytest/python.py', line 156, in pytest_pyfunc_call\n result = testfunction(**testargs)")])
brian2/tests/test_subgroup.py:128: AssertionError
============================================= warninThere was a problem hiding this comment.
Hmm, could you please change this assert statement so that we can see what the wrong warning is? I'm not getting it on my machine, so it might be something related to the compiler, for example. Try this and post what it prints:
assert (
len(l) == n_warnings
), f"expected {int(n_warnings)} for '{value}', got {len(l)} warnings: {[entry[1] for entry in l]}"There was a problem hiding this comment.
@mstimberg here
assert (
len(l) == n_warnings
), f"expected {int(n_warnings)} for '{value}', got {len(l)} warnings: {[entry[1] for entry in l]}"
E AssertionError: expected 1 for 'i', got 2 warnings: ['brian2.core.variables.ambiguous_string_expression', 'brian2.core.base.unused_brian_object']
E assert 2 == 1
E + where 2 = len([('WARNING', 'brian2.core.variables.ambiguous_string_expression', "The string expression used for setting 'v' refers t.../python3.13/site-packages/_pytest/python.py', line 156, in pytest_pyfunc_call\n result = testfunction(**testargs)")])
brian2/tests/test_subgroup.py:128: AssertionErrorwe get the extra 'brian2.core.base.unused_brian_object' error idk why
There was a problem hiding this comment.
and this is trivial , because it would be so , as we never ran or network as we don't want that here ... so maybe I can add a simple check to avoid this ?
Great Thank you for the reviews , I'll start to look into them and resolve them quickly, so we can get this finally merged :) |
…ead of python indirection
chnages done to remove python indirection from |
|
@mstimberg something happened and the whole testsuite broke down ... Is it related to the new python version release ? |
|
also @mstimberg does it make sense to have the synapses changes we discussed for |
I think I prefer to have it in a separate PR – it is a well-defined separate issue and in general, it is better to have smaller (but non-trivial) PRs. |
Yes, this is very likely related to the new Python release, since tests now run with Python 3.14. I will have a closer look, in general we should be compatible with 3.14 (we are testing things automatically once a week against the unreleased upcoming Python and numpy versions, the last run from two days ago went through without issue: https://github.com/brian-team/brian2/actions/runs/18254511895 |
|
Ok, the problem seems to be in the |
|
It should only be a matter of minutes, I think: https://redirect.github.com/actions/python-versions/pull/357 |
great then .. I may kindly ask for your final review then :) |
|
The Python 3.14 installation issue is sorted out, but there seems to be a genuine test failure related to |
|
So, I had a quick look at the test failure, and I think the issue is the one that I described here (where it was for |
|
@mstimberg , now this should pass ( hopefully 🤞) ... the bug was minor I just did the |
@mstimberg , had a yander at this while looking at the above fix ... and did now think about this ... yes ideally , I agree with you the best solution would be for us to # In Device base class
class Device:
def supports_dynamic_size_access(self):
"""Whether this device supports reading array sizes dynamically."""
return False
def get_dynamic_array_size(self, array_var):
raise NotImplementedError("This device does not support dynamic size access")and then class RuntimeDevice(Device):
def supports_dynamic_size_access(self):
return True
def get_dynamic_array_size(self, array_var):
val = self.get_value(array_var, access_data=True)
return val.shape if array_var.ndim > 1 else len(val)So then In ArrayVariable we would have : class ArrayVariable(Variable):
@property
def size(self):
"""Get size - dynamic in runtime mode, no changes in standalone."""
if (getattr(self, 'dynamic', False) and
self.device.supports_dynamic_size_access()):
return self.device.get_dynamic_array_size(self)
return self.sizeanother , high level thought would be to add this property in the Brian preferences to make this more configurable , just a thought :) |
2 participants
C++ Dynamic Arrays for JIT Replacement
As part of our ongoing effort to replace Brian’s current Cython-based just-in-time (JIT) compilation mechanism, this PR introduces the first draft implementation of new C++-based dynamic arrays for use in the runtime system.
This is an initial step toward our broader goal of replacing Python-based runtime data structures (like
DynamicArray,SpikeQueue, etc.) with fully native C++ equivalents. The long-term plan is to unify runtime and standalone modes using shared C++ templates—eliminating Cython entirely from the core simulation loop.