Add intra-population edges to test circuit, fix external edge merge bug#54
Open
cattabiani wants to merge 7 commits into
Open
Add intra-population edges to test circuit, fix external edge merge bug#54cattabiani wants to merge 7 commits into
cattabiani wants to merge 7 commits into
Conversation
…xternal edge merge - Add A->A and B->B edges to test circuit (C->C intentionally omitted to test the no-edge-file case) - Update test assertions for new external populations and edge counts - Fix bug in _copy_filtered_edges: when source_filter is active, pass only the filtered source DataFrame to _write_masked_edges to prevent duplicate index lookups when multiple sources share local node IDs
f210971 to
c52599d
Compare
The concat of multiple source DataFrames is conceptually wrong — sources come from different ID spaces and should never be merged for lookup. Now each input resolves its own source_df directly, and an assertion ensures multi-source populations always provide a source_filter.
…ulation-edges # Conflicts: # brainbuilder/utils/sonata/split_population.py
…ulation-edges # Conflicts: # brainbuilder/utils/sonata/split_population.py
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
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.
Summary
Adds intra-population edges (A→A, B→B) to the
split_subcircuittest circuit and fixes a bug in_copy_filtered_edgesexposed by the new edge topology.Test data changes
Updated
circuit_config.json,circuit_config_subcircuit.json, and regeneratededges.h5. All test assertions updated to reflect the new external populations and edge counts.Bug fix: source ID lookup in multi-source external populations
The problem
_copy_filtered_edgesused to concatenate all source DataFrames into one:This is conceptually wrong. The sources come from different ID spaces (e.g., the parent's
external_Apopulation vs the localApopulation). When two sources happen to share the same local node ID, the concatenated DataFrame has duplicate index values, and.loc[id]returns multiple rows — corrupting the edge file.Concrete example
When extracting c4 from c3_c2_c1, the
external_Apopulation has two sources:"external_A": node 2 (inherited from parent) → new_id 0"A": node 2 (newly removed) → new_id 1The concat produces index
[2, 2]. When processing the parent'sexternal_A__Cedge (src=2, tgt=3), the lookupsrc_concat.loc[2]returns both rows, appending[0, 1]to source_node_id but only[3]to target_node_id.The fix
Removed
src_concatentirely. Each input now resolves its own source DataFrame directly viasource_filter. For single-source populations (the common case), the single entry is used directly. For multi-source populations, an assertion enforces thatsource_filteris always provided.Why it wasn't triggered before
The bug requires intra-population edges (A→A) so that removed A nodes become
external_Avia the same-population path. Without them, external nodes were only discovered through inter-population edges, and the local ID collision never occurred.