From c0f96c76f92ddff211c9c95819d019e928bb3861 Mon Sep 17 00:00:00 2001 From: Christian Tabedzki <35670232+tabedzki@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:04:06 -0400 Subject: [PATCH 1/6] perf: load wiring maps once per process instead of per session Every ChannelmapGUI session called backend.make_wiring_maps(), which unpickled its own ~24 MB copy of the electrode wiring conflict maps. The result is identical across sessions and only ever read, so make_wiring_maps now resolves its argument and delegates to an lru_cache-backed loader. Every caller for a given directory, whether it passes a Path or a str, gets the same object. Split out of #52. Assisted-by: ClaudeCode:claude-opus-5-5 --- pixelmap/backend.py | 28 +++++++++++++++++++++++++++- tests/test_core_functionality.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/pixelmap/backend.py b/pixelmap/backend.py index 74178bc..5dd03c9 100644 --- a/pixelmap/backend.py +++ b/pixelmap/backend.py @@ -2,6 +2,7 @@ ## Imports ## ############# +from functools import lru_cache from pathlib import Path import pickle @@ -105,7 +106,32 @@ def find_forbidden_electrodes(selected_electrodes, wiring_df): return np.array(forbidden_electrodes).astype(int) def make_wiring_maps(wiring_maps_dir): - "Precomputes electrode wiring conflict maps for each probe type." + """ + Precomputes electrode wiring conflict maps for each probe type. + + Returns a single object SHARED across every caller (and every Panel + session) for a given wiring_maps_dir - it is memoized in-process via + functools.lru_cache, keyed on the resolved directory path (so a Path + and an equivalent str both hit the same cache entry). The unpickled + dict is ~24.2 MB and identical for every session; before this caching + was added, each session re-unpickled its own private copy (measured: + 30.2 MB retained per session, 10 concurrent sessions = +350 MB RSS). + + IMPORTANT: because the mapping is shared, callers MUST treat it as + read-only - copy `.keys()` instead of mutating them, and never mutate + the conflicting-electrodes sets in place (e.g. no `|=`/`-=`/`.add()` + on the returned sets). The GUI's `Electrodes` class only ever reads + this mapping (copies `.keys()`, indexes `wiring_map[electrode]`), so + it is safe to share. + """ + wiring_maps_dir = Path(wiring_maps_dir).resolve() + assert wiring_maps_dir.exists(), f"{wiring_maps_dir} does not exist!" + return _load_wiring_maps(str(wiring_maps_dir)) + + +@lru_cache(maxsize=None) +def _load_wiring_maps(wiring_maps_dir): + "Loads (or builds) the pickled electrode wiring conflict maps for each probe type. Process-wide cache, keyed on the resolved directory path; use ._load_wiring_maps.cache_clear() in tests that mutate a directory's wiring files." wiring_maps_dir = Path(wiring_maps_dir) assert wiring_maps_dir.exists(), f"{wiring_maps_dir} does not exist!" cache_file = wiring_maps_dir / "wiring_maps.pkl" diff --git a/tests/test_core_functionality.py b/tests/test_core_functionality.py index ae8d5cd..c06c8ee 100644 --- a/tests/test_core_functionality.py +++ b/tests/test_core_functionality.py @@ -16,6 +16,8 @@ get_electrodes, find_forbidden_electrodes, _verify_hardware_violations, + make_wiring_maps, + _load_wiring_maps, ) from pixelmap.utils.imro import ( save_to_imro_file, @@ -269,3 +271,33 @@ def test_multiple_presets_same_probe(self, wiring_maps_dir, tmp_path): save_to_imro_file(imro_list, str(output_file)) assert output_file.exists() + + +class TestWiringMapsSharing: + """make_wiring_maps must return one shared, process-wide object per + directory instead of unpickling a fresh copy on every call.""" + + def test_make_wiring_maps_returns_same_object_for_path_and_str(self, wiring_maps_dir): + by_path = make_wiring_maps(wiring_maps_dir) + by_str = make_wiring_maps(str(wiring_maps_dir)) + + assert by_path is by_str + + # Sanity: still contains every probe type in WIRING_FILE_MAP. + assert set(by_path.keys()) == set(WIRING_FILE_MAP.keys()) + + def test_make_wiring_maps_returns_same_object_across_calls(self, wiring_maps_dir): + first = make_wiring_maps(wiring_maps_dir) + second = make_wiring_maps(wiring_maps_dir) + + assert first is second + + def test_make_wiring_maps_cache_clear_forces_reload(self, wiring_maps_dir): + """Tests (or callers) that mutate a wiring_maps_dir between calls + must clear the cache to see a fresh reload.""" + first = make_wiring_maps(wiring_maps_dir) + _load_wiring_maps.cache_clear() + second = make_wiring_maps(wiring_maps_dir) + + assert first is not second + assert set(first.keys()) == set(second.keys()) From 0d845c4503ccd502aa1447ab50f4dfe6ac40268b Mon Sep 17 00:00:00 2001 From: Christian Tabedzki <35670232+tabedzki@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:14:36 -0400 Subject: [PATCH 2/6] refactor: use functools.cache for the wiring maps loader lru_cache(maxsize=None) is what functools.cache already spells. Assisted-by: ClaudeCode:claude-opus-5-5 --- pixelmap/backend.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pixelmap/backend.py b/pixelmap/backend.py index 5dd03c9..486d252 100644 --- a/pixelmap/backend.py +++ b/pixelmap/backend.py @@ -2,7 +2,7 @@ ## Imports ## ############# -from functools import lru_cache +from functools import cache from pathlib import Path import pickle @@ -111,7 +111,7 @@ def make_wiring_maps(wiring_maps_dir): Returns a single object SHARED across every caller (and every Panel session) for a given wiring_maps_dir - it is memoized in-process via - functools.lru_cache, keyed on the resolved directory path (so a Path + functools.cache, keyed on the resolved directory path (so a Path and an equivalent str both hit the same cache entry). The unpickled dict is ~24.2 MB and identical for every session; before this caching was added, each session re-unpickled its own private copy (measured: @@ -129,7 +129,7 @@ def make_wiring_maps(wiring_maps_dir): return _load_wiring_maps(str(wiring_maps_dir)) -@lru_cache(maxsize=None) +@cache def _load_wiring_maps(wiring_maps_dir): "Loads (or builds) the pickled electrode wiring conflict maps for each probe type. Process-wide cache, keyed on the resolved directory path; use ._load_wiring_maps.cache_clear() in tests that mutate a directory's wiring files." wiring_maps_dir = Path(wiring_maps_dir) From 9c1179012a92892359c2e64b2bc3dab84a84937a Mon Sep 17 00:00:00 2001 From: Christian Tabedzki <35670232+tabedzki@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:14:50 -0400 Subject: [PATCH 3/6] refactor: drop the duplicate wiring maps directory check make_wiring_maps already asserts that the resolved directory exists before calling _load_wiring_maps, so the loader's own check never fires. Assisted-by: ClaudeCode:claude-opus-5-5 --- pixelmap/backend.py | 1 - tests/test_core_functionality.py | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pixelmap/backend.py b/pixelmap/backend.py index 486d252..710e812 100644 --- a/pixelmap/backend.py +++ b/pixelmap/backend.py @@ -133,7 +133,6 @@ def make_wiring_maps(wiring_maps_dir): def _load_wiring_maps(wiring_maps_dir): "Loads (or builds) the pickled electrode wiring conflict maps for each probe type. Process-wide cache, keyed on the resolved directory path; use ._load_wiring_maps.cache_clear() in tests that mutate a directory's wiring files." wiring_maps_dir = Path(wiring_maps_dir) - assert wiring_maps_dir.exists(), f"{wiring_maps_dir} does not exist!" cache_file = wiring_maps_dir / "wiring_maps.pkl" # If pickled result found and covers all current probe types, preload it diff --git a/tests/test_core_functionality.py b/tests/test_core_functionality.py index c06c8ee..069bde2 100644 --- a/tests/test_core_functionality.py +++ b/tests/test_core_functionality.py @@ -301,3 +301,7 @@ def test_make_wiring_maps_cache_clear_forces_reload(self, wiring_maps_dir): assert first is not second assert set(first.keys()) == set(second.keys()) + + def test_make_wiring_maps_rejects_missing_directory(self, tmp_path): + with pytest.raises(AssertionError, match="does not exist"): + make_wiring_maps(tmp_path / "missing") From c56c61a3b4c43a720d1fbaa71b6385711d83046a Mon Sep 17 00:00:00 2001 From: Christian Tabedzki <35670232+tabedzki@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:15:18 -0400 Subject: [PATCH 4/6] test: clear the wiring maps cache around each sharing test The sharing tests assert object identity, so a map cached by an earlier test (or left behind by one) could make them pass or fail depending on test order. Assisted-by: ClaudeCode:claude-opus-5-5 --- tests/test_core_functionality.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/test_core_functionality.py b/tests/test_core_functionality.py index 069bde2..f85e8ff 100644 --- a/tests/test_core_functionality.py +++ b/tests/test_core_functionality.py @@ -277,6 +277,13 @@ class TestWiringMapsSharing: """make_wiring_maps must return one shared, process-wide object per directory instead of unpickling a fresh copy on every call.""" + @pytest.fixture(autouse=True) + def _isolated_cache(self): + """Start and end each test with an empty cache so no test sees another's map.""" + _load_wiring_maps.cache_clear() + yield + _load_wiring_maps.cache_clear() + def test_make_wiring_maps_returns_same_object_for_path_and_str(self, wiring_maps_dir): by_path = make_wiring_maps(wiring_maps_dir) by_str = make_wiring_maps(str(wiring_maps_dir)) From 586655144a708008f4a3adce2f6337583a82d97e Mon Sep 17 00:00:00 2001 From: Christian Tabedzki <35670232+tabedzki@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:23:20 -0400 Subject: [PATCH 5/6] fix: make the shared wiring maps read-only make_wiring_maps now returns one object shared by every session, so an in-place mutation (e.g. |= or .add() on a conflict set) would silently change electrode availability for everyone. Return MappingProxyType views with frozenset values instead, so mutating raises an error. The pickle on disk stays plain dicts and sets. Load time and peak RSS are unchanged. The Electrodes annotation now accepts any read-only mapping. Assisted-by: ClaudeCode:claude-opus-5-5 --- pixelmap/backend.py | 15 ++++++-- pixelmap/gui/gui.py | 3 +- tests/test_core_functionality.py | 62 ++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/pixelmap/backend.py b/pixelmap/backend.py index 710e812..24df20a 100644 --- a/pixelmap/backend.py +++ b/pixelmap/backend.py @@ -5,6 +5,7 @@ from functools import cache from pathlib import Path import pickle +from types import MappingProxyType import matplotlib.cm as cm import matplotlib.colors as mcolors @@ -141,7 +142,7 @@ def _load_wiring_maps(wiring_maps_dir): with open(cache_file, 'rb') as f: wiring_maps = pickle.load(f) if set(wiring_maps.keys()) == expected_keys: - return wiring_maps + return _freeze_wiring_maps(wiring_maps) # Cache is stale (probe types changed) — rebuild below # Generate wiring maps @@ -165,7 +166,17 @@ def _load_wiring_maps(wiring_maps_dir): with open(cache_file, 'wb') as f: pickle.dump(wiring_maps, f) - return wiring_maps + return _freeze_wiring_maps(wiring_maps) + + +def _freeze_wiring_maps(wiring_maps): + "Read-only view of the wiring maps, so the copy shared by every session can't be mutated." + return MappingProxyType({ + probe_type: MappingProxyType({ + electrode: frozenset(conflicting) for electrode, conflicting in conflicts.items() + }) + for probe_type, conflicts in wiring_maps.items() + }) def format_wiring_df(wiring_df): diff --git a/pixelmap/gui/gui.py b/pixelmap/gui/gui.py index 025e15d..93b857d 100644 --- a/pixelmap/gui/gui.py +++ b/pixelmap/gui/gui.py @@ -12,6 +12,7 @@ import logging import re import threading +from collections.abc import Mapping, Set as AbstractSet from dataclasses import dataclass, field from io import BytesIO, StringIO from pathlib import Path @@ -133,7 +134,7 @@ def _darken_hex(hex_color: str, factor: float = 0.6) -> str: class Electrodes: """Manages electrode selection state.""" - wiring_map: dict[Electrode, set[Electrode]] + wiring_map: Mapping[Electrode, AbstractSet[Electrode]] n_maximum_electrodes: int # TODO: implement per-shank maximum for NXTs available: set[Electrode] = field(default_factory=set) selected: set[Electrode] = field(default_factory=set) diff --git a/tests/test_core_functionality.py b/tests/test_core_functionality.py index f85e8ff..21ffc32 100644 --- a/tests/test_core_functionality.py +++ b/tests/test_core_functionality.py @@ -11,6 +11,7 @@ import pytest import numpy as np from pathlib import Path +import shutil from pixelmap.backend import ( get_electrodes, @@ -19,6 +20,7 @@ make_wiring_maps, _load_wiring_maps, ) +from pixelmap.gui.gui import Electrodes from pixelmap.utils.imro import ( save_to_imro_file, read_imro_file, @@ -312,3 +314,63 @@ def test_make_wiring_maps_cache_clear_forces_reload(self, wiring_maps_dir): def test_make_wiring_maps_rejects_missing_directory(self, tmp_path): with pytest.raises(AssertionError, match="does not exist"): make_wiring_maps(tmp_path / "missing") + + def test_make_wiring_maps_is_read_only(self, wiring_maps_dir): + """The map is shared by every session, so mutating it must fail loudly.""" + wiring_maps = make_wiring_maps(wiring_maps_dir) + conflicts = wiring_maps["1.0"] + electrode, conflicting = next((e, c) for e, c in conflicts.items() if c) + + with pytest.raises(TypeError): + wiring_maps["1.0"] = {} + with pytest.raises(TypeError): + conflicts[electrode] = set() + with pytest.raises(TypeError): + del conflicts[electrode] + with pytest.raises(AttributeError): + conflicting.add(electrode) + with pytest.raises(AttributeError): + conflicting.clear() + + def test_electrodes_selection_leaves_shared_map_untouched(self, wiring_maps_dir): + wiring_map = make_wiring_maps(wiring_maps_dir)["1.0"] + electrode, conflicting = next((e, c) for e, c in wiring_map.items() if c) + snapshot = {e: set(c) for e, c in wiring_map.items()} + + first = Electrodes(wiring_map, n_maximum_electrodes=384) + second = Electrodes(wiring_map, n_maximum_electrodes=384) + first.push_undo_snapshot() + first.select(electrode) + + assert electrode in first.selected + assert conflicting <= first.unavailable + assert isinstance(first.available, set) and isinstance(first.unavailable, set) + assert second.selected == set() and second.unavailable == set() + + first.undo() + assert first.selected == set() and first.unavailable == set() + first.redo() + first.deselect(electrode) + assert first.selected == set() and first.unavailable == set() + assert {e: set(c) for e, c in wiring_map.items()} == snapshot + + @pytest.mark.filterwarnings("ignore::FutureWarning") + def test_rebuilt_wiring_maps_are_pickled_and_reloaded(self, wiring_maps_dir, tmp_path, monkeypatch): + """A rebuild must still write a loadable pickle, and both paths return the read-only map.""" + # Rebuilding every probe type takes minutes; one probe exercises the same code. + file_map = {"1.0": WIRING_FILE_MAP["1.0"]} + monkeypatch.setattr("pixelmap.backend.WIRING_FILE_MAP", file_map) + for filename in file_map["1.0"]: + shutil.copy(wiring_maps_dir / filename, tmp_path / filename) + + built = make_wiring_maps(tmp_path) + assert (tmp_path / "wiring_maps.pkl").exists() + _load_wiring_maps.cache_clear() + loaded = make_wiring_maps(tmp_path) + + assert built is not loaded + for wiring_maps in (built, loaded): + assert set(wiring_maps) == {"1.0"} + with pytest.raises(TypeError): + wiring_maps["1.0"] = {} + assert dict(built["1.0"]) == dict(loaded["1.0"]) From 9fe6bcea0a905250e700574620f9d969ddd6fdec Mon Sep 17 00:00:00 2001 From: Christian Tabedzki <35670232+tabedzki@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:23:38 -0400 Subject: [PATCH 6/6] docs: trim the wiring maps docstrings Keep the contract (cached per directory, shared, read-only) and leave the measurements and history to the commit log and PR. The read-only rule no longer needs spelling out now that the mapping enforces it. Assisted-by: ClaudeCode:claude-opus-5-5 --- pixelmap/backend.py | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/pixelmap/backend.py b/pixelmap/backend.py index 24df20a..48c871f 100644 --- a/pixelmap/backend.py +++ b/pixelmap/backend.py @@ -110,20 +110,8 @@ def make_wiring_maps(wiring_maps_dir): """ Precomputes electrode wiring conflict maps for each probe type. - Returns a single object SHARED across every caller (and every Panel - session) for a given wiring_maps_dir - it is memoized in-process via - functools.cache, keyed on the resolved directory path (so a Path - and an equivalent str both hit the same cache entry). The unpickled - dict is ~24.2 MB and identical for every session; before this caching - was added, each session re-unpickled its own private copy (measured: - 30.2 MB retained per session, 10 concurrent sessions = +350 MB RSS). - - IMPORTANT: because the mapping is shared, callers MUST treat it as - read-only - copy `.keys()` instead of mutating them, and never mutate - the conflicting-electrodes sets in place (e.g. no `|=`/`-=`/`.add()` - on the returned sets). The GUI's `Electrodes` class only ever reads - this mapping (copies `.keys()`, indexes `wiring_map[electrode]`), so - it is safe to share. + The result is cached per resolved directory, so every caller (and every + Panel session) shares one read-only mapping instead of unpickling its own. """ wiring_maps_dir = Path(wiring_maps_dir).resolve() assert wiring_maps_dir.exists(), f"{wiring_maps_dir} does not exist!" @@ -132,7 +120,7 @@ def make_wiring_maps(wiring_maps_dir): @cache def _load_wiring_maps(wiring_maps_dir): - "Loads (or builds) the pickled electrode wiring conflict maps for each probe type. Process-wide cache, keyed on the resolved directory path; use ._load_wiring_maps.cache_clear() in tests that mutate a directory's wiring files." + "Loads (or builds and pickles) the wiring conflict maps. Call cache_clear() after changing a directory's wiring files." wiring_maps_dir = Path(wiring_maps_dir) cache_file = wiring_maps_dir / "wiring_maps.pkl"