Skip to content

Fix _float_connection_matrix silently overwriting values for multi-synapse connections#79

Closed
Sanchit2662 wants to merge 1 commit intobrian-team:masterfrom
Sanchit2662:fix/float-connection-matrix-multisynapse
Closed

Fix _float_connection_matrix silently overwriting values for multi-synapse connections#79
Sanchit2662 wants to merge 1 commit intobrian-team:masterfrom
Sanchit2662:fix/float-connection-matrix-multisynapse

Conversation

@Sanchit2662
Copy link
Copy Markdown

I was looking through the plotting utilities and found a bug in _float_connection_matrix that silently produces wrong output for networks with multiple synapses between the same source-target pair.

The original code does a plain NumPy fancy index assignment:

  full_matrix[targets - np.min(targets), sources - np.min(sources)] = values                                                                                         

When you have multi-synaptic connections, multiple entries in sources and targets point to the same matrix cell. NumPy just overwrites each time, so only the last synapse's value survives. If you had two synapses between neuron 0 and neuron 1 with weights 0.3 and 0.7, the matrix would show 0.7 and the 0.3 is gone with no warning.

I fixed it by replacing the assignment with two np.add.at accumulations one summing the values, one counting how many synapses hit each cell then dividing to get the mean. Mean makes sense here because the matrix is displaying a synaptic property like weight or delay per connection pair, so the average across al synapses between that pair is the meaningful thing to show.

I also updated the docstring to mention that multi-synaptic pairs display the mean, so users know what they're looking at.

…napse connections

Signed-off-by: Sanchit2662 <sanchit2662@gmail.com>
@mstimberg
Copy link
Copy Markdown
Member

Thanks for the PR @Sanchit2662. I'm not quite sure about this one, though. The _float_connection_matrix function is an internal function, not something a user would use. The user-facing function plot_synapses does not silently do the wrong thing here, but raises an error that this is only supported for "hexbin" plots (which bin synapses across multiple synapses already, so there is no issue). In a way, the new situation would be more misleading, since it silently takes a mean instead of raising an error. I'm not convinced that taking the mean is always the best choice here. E.g. for synaptic weights, the sum would be more natural. I therefore tend to rather stick to the current solution which raises a clear error.

@Sanchit2662
Copy link
Copy Markdown
Author

Thanks for pointing that out, I missed that plot_synapses already handles this at the public API level with the NotImplementedError. So patching the internal function doesn't really solve anything meaningful since it's never actually reached with multi-synapse values through the normal flow.
I'll close this one. If the guard ever gets relaxed down the line, the aggregation semantics can be decided properly at that point rather than silently averaging here.

@Sanchit2662 Sanchit2662 closed this Apr 1, 2026
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