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 😉 |
| * outside the logical size. To actually release unused memory, | ||
| * call `shrink_to_fit()`. | ||
| */ | ||
| void resize(size_t new_rows, size_t new_cols) |
There was a problem hiding this comment.
Let's first make sure that things work, but I wonder if it wouldn't be a good time to significantly simplify the code: if I am not mistaken, the only code that uses DynamicArray2D is the StateMonitor and this one will always resize along the first dimension. With the right memory layout, this would mean that we always add new data in the end, and therefore we'd never have to save the stride, for example. Something to keep in mind before finalizing the PR!
There was a problem hiding this comment.
@Legend101Zz As I mentioned above, I think it would significantly simplify the code here to only support resize_along_first – the more general resize function is never used in practice.
…r older mode so some new features are not available
| m_data.shrink_to_fit(); | ||
| } | ||
| size_t size() const noexcept { return m_size; } | ||
| size_t capacity() const noexcept { return m_data.size(); } |
There was a problem hiding this comment.
sadly we cannot use these methods I guess due to some of the compilers running on old cpp versions :(
There was a problem hiding this comment.
I kept getting this error in ci
|
All tests pass now :) |
|
@mstimberg it finally passes 😭😭🥳 |
|
@mstimberg as discussed, I’m starting to clean up the logs and other unnecessary code. I’ll begin by removing just the logs first ( 42a7c3f ), and then gradually move on to the other pieces. This way, we can easily spot if (hopefully not please 😭) the test suite breaks at any point. |
|
@mstimberg also we got a conflict in workflow file , I guess due to recent changes to 
|
|
Hmm pretty strange somehow the above two commits break a testcase for all Something related to cython not building the dynamic arrays nicely ? |
ahh sorry my bad 😅 
did not see the message down below the PR that merged the changes from main had introduced a lot of changes so maybe something from there broke it ... |
|
Hmm, yes, it seems to fail on the |
And interestingly if I remove this line Then the test passes ... 
|
|
I think the problem is that this function return 0 where it shouldn't, but I won't have the time to look into this in more detail today: brian2/brian2/memory/cythondynamicarray.pyx Lines 154 to 166 in e16ac73 |
|
found the clue ... print('test',spikes_all,spikes_all.i,spikes_all.num_spikes)
> test <SpikeMonitor, recording from 'spikemonitor'> <spikemonitor.i: array([0, 4, 5], dtype=int32)> 3so means that dynamic array's shape property and data property are out of sync ... |
|
So I did some debugging
this seems to work fine ... infact the whole @property
def data(self):
"""Return numpy array view of underlying data"""
cdef cnp.npy_intp shape[1]
cdef size_t size = self.get_size()
cdef void* data_ptr = self.get_data_ptr()
print(f'data property: size={size}, data_ptr={<long>data_ptr}')
shape[0] = size
if size == 0:
print('Returning empty array')
return np.array([], dtype=self.dtype)
# Create the numpy array
result = cnp.PyArray_SimpleNewFromData(1, shape, self.numpy_type, data_ptr)
print(f'Created numpy array: shape={result.shape}, dtype={result.dtype}')
print(f'Array contents: {result}')
return result
But problem is somehow the end 
|
|
will look indepth tomorrow |
|
Phew, this is actually a non-trivial issue… The reason why things broke is that the abstract |
|
I just pushed the change I suggested in my comment above (adding |
Great understood ( also tests pass now so I guess everything works fine ) ... I still need to understand and make sense of what you said 😅 , I guess a few debug points should make it easier ( will have to change the IDE back to vsc ) , but is this intermediate fix good for us for now ? So should I continue with my part on cleaning up the PR ? |
Yes, this is good enough for now (at least this PR) – but I think we will need the same kind of fix for |
…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.
|
|
||
| def get_capsule_type(var): | ||
| """Get the capsule type name for PyCapsule_GetPointer""" | ||
| if not hasattr(var, "ndim"): |
There was a problem hiding this comment.
I think this check is not necessary here – this is a function that is only called internally, and should therefore always be called with a var of the correct type. If not, we'll get an AttributeError which gives us basically the same information.
| if isinstance(var, DynamicArrayVariable): | ||
| # We're dealing with a dynamic array (like synaptic connections that grow during simulation) | ||
| # For these arrays, we want BLAZING FAST access, so we'll create direct C++ pointers | ||
| # This bypasses all Python overhead and gives us pure C++ speed! |
There was a problem hiding this comment.
Not a big deal, obviously, but this comment feels a bit out of place and more like "notes to self" or a blog post.
| if get_dtype_str(var.dtype) == "bool": | ||
| if isinstance(var, DynamicArrayVariable): | ||
| # For Dynamic Arrays, we get the data pointer directly from the C++ object | ||
| # This works for all types, including bools, because the C++ class handles the type correctly. |
There was a problem hiding this comment.
Not sure I understand this comment – two lines below bool is special-cased, no?
| ), | ||
| ] | ||
| if getattr(var, "ndim", 1) > 1: | ||
| continue # multidimensional (dynamic) arrays have to be treated differently |
There was a problem hiding this comment.
We are in the else block here, so the array should not be dynamic, or am I mistaken?
| ] | ||
| if getattr(var, "ndim", 1) > 1: | ||
| continue # multidimensional (dynamic) arrays have to be treated differently | ||
| if get_dtype_str(var.dtype) == "bool": |
There was a problem hiding this comment.
Given my above comment that we are dealing with a non-dynamic array here, do we really need to special-case bool?
| inline T &operator()(int i, size_t j) { return operator()(static_cast<size_t>(i), j);} | ||
| /** | ||
| * @brief Returns a copy of row i as std::vector<T>. | ||
| * @note This is a copy; for slicing without copy, consider returning a view. |
There was a problem hiding this comment.
Who is this comment addressing?
| cdef int flags = cnp.NPY_ARRAY_WRITEABLE | ||
|
|
||
| # A little optimization: if the memory is perfectly packed (no extra space in rows), | ||
| # we can tell NumPy it's "C-contiguous". This can speed up some operations. |
There was a problem hiding this comment.
Isn't this always the case, since it is how we construct things? I think stride just returns the number of columns
|
|
||
| extensions.append(spike_queue_ext) | ||
|
|
||
| dyanamic_array_ext = require_cython_extension( |
| dyanamic_array_ext = require_cython_extension( | ||
| module_path=["brian2", "memory"], | ||
| module_name="cythondynamicarray", | ||
| extra_include_dirs=["brian2/devices/cpp_standalone/brianlib"] |
There was a problem hiding this comment.
I might be mistaken, but do we not specify the extra include dir three times? Once here, once in the header of the Cython file, and once in extension_manager.py?
| 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?
Great Thank you for the reviews , I'll start to look into them and resolve them quickly, so we can get this finally merged :) |
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.