event handling: fix a bug and clean house #2421
Conversation
boeschf
commented
Oct 16, 2024
boeschf
commented
- fix event stream when more than one cell in a cell group have same synapse
- events would previously no longer necessarily be sorted by time
- in order to simplify: also sort with respect to mechanism index (as was previously only required for the gpu backend)
- add pertinent test
- while cleaning up: overhauled the event related files and data structures
- removed dead code
- made event handling less generic (this feature was not used anywhere)
| * | ||
| * Time values must be well ordered with respect to `operator>`. | ||
| */ | ||
|
|
||
| template <typename Event> | ||
| struct default_event_time { |
There was a problem hiding this comment.
No criticism, just curiosity: Why choose this over a simple function?
Also, potentially we know that time must be of type arb_time_type, no?
Finally, might a concept help clean up things more?
| v.spike_counter_.clear(); | ||
| v.spike_counter_.resize(steps.size(), 0); | ||
| v.spikes_.clear(); | ||
| v.spikes_.reserve(v.ev_data_.capacity()); |
There was a problem hiding this comment.
We are using capacity here, that seems unusual.
There was a problem hiding this comment.
this is intentional, as ev_data_ has been cleared before. I should probably make a comment. Otherwise, I can reorder the operations, so that v.clear() is called afterwards.
There was a problem hiding this comment.
I think that would be confusing, just make a note.
arbor/backends/event_stream_base.hpp
Outdated
| auto first = stream.spikes_.begin(); | ||
| auto last = stream.spikes_.end(); | ||
| auto pivot = std::prev(last, 1); | ||
| std::rotate(std::upper_bound(first, pivot, *pivot, [](auto const& l, auto const& r) noexcept { |
There was a problem hiding this comment.
Since #2401 this should be simply < with all the same semantics and performance, no?
| arb_size_type step = 0; | ||
| time_type time = 0; | ||
| cell_local_size_type mech_index = 0; | ||
| float weight = 0; |
There was a problem hiding this comment.
| float weight = 0; | |
| float weight = 0; | |
| auto operator<=>(const spike_data&) const = default; |
There was a problem hiding this comment.
absolutely, thanks for pointing this out
Since we need sorting in any case due to the events not being in order w.r.t. time under certain conditions (when more than one cell in a cell group have same synapse) I just rolled the sorting w.r.t. |
|
Understoo! |
|
I'll merge this now, correctness is more important, we can address potential performance later. |
