Vectorize line-source weight computation (2)#82
Open
cattabiani wants to merge 28 commits into
Open
Conversation
Adapt all example simulation configs to the updated libsonata API: - Remove electrodes_file from the run section - Add electrodes_file to each LFP report block - Remove variable_name from LFP reports (no longer allowed)
- Pick the node manager with cells instead of asserting exactly one - Use alternate_morphology_formats (neurolucida-asc, h5v1) when available
Replace per-segment Python loop in _get_coeffs_line_source and per-electrode loop in get_weights with fully vectorized numpy operations. - Add _precompute_segment_geometry helper for array extraction - Vectorize h/l/r2 computation with np.where for sign cases - Add _get_coeffs_line_source_batch for multi-electrode broadcasting - Batch consecutive LINE_SOURCE electrodes in get_weights - Chunk by 50 electrodes to bound memory at ~4 GB peak - Add verbose progress logging (rank 0 only) - Add scalar-vs-vectorized comparison test (rtol=1e-10)
neurodamus-models 3.0+ removed CMakeLists.txt. Replace the cmake-based build with nrnivmodl, following the approach used in neurodamus's own CI. Also bump NEURODAMUS_COMMIT to 4.2.3.
tests/mechanisms/neocortex/ was added after the 4.2.3 tag, so cloning at that tag fails. Clone at main (default) since these are just standard .mod files not version-sensitive.
neurodamus-models/mod/ contains files (InhPoissonStim, vecevent) that duplicate mechanisms already in neurodamus core data/mod/. Matching neurodamus's own CI approach: only combine core mods + neocortex test mechanisms. No need to separately copy neurodamus-models/mod/.
SonataReportHelper.mod and SonataReports.mod require bbp/sonata/reports.h from libsonatareport, which is not installed. These mods are not needed for BlueRecording tests (the old CI used -DNEURODAMUS_ENABLE_REPORTING=OFF).
Keep SonataReport mods in the build (needed for neurodamus checks) but disable the reporting lib dependency via compiler flag.
This branch has the electrodes_file fix (no longer reads from Run object) and includes tests/mechanisms/neocortex/. Will be pinned to a tag once the next neurodamus release lands.
…line-source-weights
Adds electrodes_line_source.csv (3 LineSource electrodes) and a reference weights file for the rat_s1_forelimb_l56_10cells circuit. The reference was generated from the pre-vectorization code. Also includes the circuit.py change to use nd._sonata_circuits for morphology path resolution.
neurodamus already resolves alternate morphology formats in its circuit config parsing. Use MorphologyPath directly instead of duplicating the logic.
Move all coefficient computation functions (line-source, point-source, reciprocity, CSD) and SegmentGeometry into bluerecording/physics.py. weights.py keeps orchestration, H5 I/O, and electrode parsing. Backward-compatible aliases are re-exported from weights.py so existing test imports continue to work.
…etry.from_positions)
Tests now import physics functions from bluerecording.physics with their proper public names.
…_coeffs_batch - Add _point_source_coeffs_batch as the shared DRY helper - Add get_coeffs_point_source_batch for multi-electrode point source - Line-source batch now reuses _point_source_coeffs_batch for soma segments - get_weights batches consecutive PointSource electrodes (same as LineSource)
…utive get_weights now groups all electrodes by type and sigma upfront, batches each group in one shot, then places results back in original electrode order. Removes the old consecutive-walking _collect_and_compute_* helpers. This handles interleaved electrode types correctly (e.g. LineSource, PointSource, LineSource all get batched properly).
Verifies that get_weights correctly groups electrodes by (type, sigma), computes batches, and reassembles results in original order. Compares against per-electrode individual computation.
Batch functions now accept sigma as a per-electrode array. get_weights groups only by type (LineSource vs PointSource), passes the sigma array for that group, and places results back in original order. No need to sub-group by sigma value anymore.
cattabiani
changed the title
cattabiani
changed the base branch from
katta/per-report-electrodes-file
to
main
mgeplf
reviewed
Jun 29, 2026
mgeplf
left a comment
mgeplf
left a comment
There was a problem hiding this comment.
only made it so far today, I'm very unfamiliar with this code, so not sure how useful I can be
There was a problem hiding this comment.
why not move the tests of physics.py out to a test_physics.py?
|
|
||
| Dispatches to the appropriate coefficient function based on each | ||
| electrode's type and returns the concatenated result. | ||
| Groups electrodes by (type, sigma), computes each group as a batch, |
There was a problem hiding this comment.
how much does computes each group as a batch actually save? how many electrodes do we expect? there seems to be a lot of complexity to batch and unbatch them, and I'm not convinced we want that complexity - however, having some numbers could convince me
| electrode_type = electrode.type | ||
| other_indices.append(idx) | ||
|
|
||
| # --- Batch compute LINE_SOURCE --- |
There was a problem hiding this comment.
these comments, as usual, don't tell me anything that the if line_source_indices doesn't already
|
|
||
| elif electrode_type is ElectrodeType.DIPOLE_RECIPROCITY: | ||
| center = mid_positions.mean(axis=1) | ||
| coeffs = get_coeffs_dipole_reciprocity(mid_positions, path_to_fields[reciprocity_idx], center) |
There was a problem hiding this comment.
why doesn't get_coeffs_dipole_reciprocity calculate the center?
|
|
||
|
|
||
| def test_get_coeffs_line_source(): | ||
| def testget_coeffs_line_source(): |
There was a problem hiding this comment.
why no underscore between test and get?
| columns = weights.columns | ||
| node_ids = np.unique(cols[:, 0]) | ||
|
|
||
| from bluerecording.weights import _get_segment_midpts |
|
|
||
| start_pos: np.ndarray # (N_line_segments, 3) — segment start positions (µm) | ||
| end_pos: np.ndarray # (N_line_segments, 3) — segment end positions (µm) | ||
| seg_lengths: np.ndarray # (N_line_segments,) — lengths in meters |
There was a problem hiding this comment.
why is this in meters when everything else is in um?
|
|
||
| def precompute_segment_geometry(positions: pd.DataFrame) -> SegmentGeometry: | ||
| """Precompute segment geometry arrays. Use ``SegmentGeometry.from_positions()`` instead.""" | ||
| return SegmentGeometry.from_positions(positions) |
There was a problem hiding this comment.
is this necessary? I couldn't find the tests for SegmentGeometry::from_positions since there is this wrapper
| positions: DataFrame of segment boundary positions (µm), shape | ||
| ``(3, N_columns)`` with MultiIndex columns ``(gid, section_id)``. | ||
| """ | ||
| n_cols = len(positions.columns) |
There was a problem hiding this comment.
why is this df arranged where the number of columns is unbounded? I find it very strange; it makes the manipulations much harder, imo, as opposed the "traditional" layout.
| ) -> float: | ||
| """Compute the line-source coefficient for a single segment. | ||
|
|
||
| All positions are in µm and are converted to m internally. |
There was a problem hiding this comment.
converted to m internally why? if the natural scale is micro, seems strange to change it
Not sure why this internal detail is described in the docstring
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces per-segment and per-electrode Python loops with vectorized numpy operations.
fix: #79
Changes
_precompute_segment_geometry: extracts segment arrays from positions DataFrame once_get_coeffs_line_source: vectorized h/l/r2 + np.where for sign cases (no loop)_get_coeffs_line_source_batch: multi-electrode broadcasting with chunking (50 at a time, ~4 GB peak)get_weights: batches consecutive LINE_SOURCE electrodes sharing the same sigmaverbose=TrueTesting