Review fixes for PR #65: map_channel_to_electrode, LFP boundaries, STTC, ImpedanceMeasurements

[MilagrosMarin]

Overview

This PR contains reviewed fixes for PR #65 (Changes to Ephys Schema by @judewerth). All of Jude's original changes are included here with the following corrections applied.

---

Fixes Applied

A — Lazy imports for neo, quantities, elephant

import neo, import quantities as pq, and from elephant.spike_train_correlation import spike_time_tiling_coefficient were moved from module-level to lazy imports inside STTC.make().

Why: These are optional heavy dependencies used only by STTC.make(). Importing them at module level causes ephys_no_curation to fail to import entirely on any environment where these packages are not installed (HPC nodes, minimal containers, dev machines not running spike sorting) — breaking LFP, frame analysis, and everything else that depends on this module. This follows the same lazy-import pattern applied to spikeinterface in the workflow repo.

---

B — map_channel_to_electrode: restricted fetch + conflict detection

# Before (unrestricted — fetches ALL probe configs):
electrode_mapping, channel_mapping = probe.ElectrodeConfig.Electrode.fetch("electrode", "channel_idx")

# After:
electrode_mapping, channel_mapping = (
    probe.ElectrodeConfig.Electrode & {"probe_type": probe_type}
).fetch("electrode", "channel_idx")

pairs = np.unique(np.column_stack([electrode_mapping, channel_mapping]), axis=0)
if len(pairs) != len(np.unique(pairs[:, 0])):
    raise ValueError("Conflicting channel-electrode mappings found ...")
electrode_mapping, channel_mapping = pairs[:, 0], pairs[:, 1]

Why: The original fetch was completely unrestricted — it pulled mappings from all electrode configs in the database regardless of probe_type. This works accidentally while only one probe type is in the database, but silently returns a wrong mixed-probe mapping the moment a second probe type is added.

probe_type is already part of ElectrodeConfig.Electrode's primary key (via -> ProbeType.Electrode), so no join through ElectrodeConfig is needed. The np.unique deduplication handles the case of multiple configs for the same probe type that share an identical mapping. If two configs have conflicting channel_idx values for the same electrode, a ValueError is raised explicitly instead of silently overwriting with a non-deterministic value.

---

C — LFP boundary trimming: elif bug for single-file sessions

# Before (elif skips end trim when session lives in a single file):
if file_relpath == file_paths[0]:
    lfps = lfps[:, start_idx:]
elif file_relpath == file_paths[-1]:   # never runs when first == last
    lfps = lfps[:, :end_idx]

# After (two independent if blocks, single slice):
start_idx = 0
end_idx = lfps.shape[1]
is_first_file = file_relpath == file_paths[0]
is_last_file = file_relpath == file_paths[-1]
if is_first_file or is_last_file:
    file_start = datetime.strptime(...)  # parsed once
    if is_first_file:
        start_idx = int((key['start_time'] - file_start).total_seconds() * fs)
    if is_last_file:
        end_idx = int((key['end_time'] - file_start).total_seconds() * fs)
lfps = lfps[:, start_idx:end_idx]

Why: When a session lives entirely within one .rhd file, file_paths[0] == file_paths[-1]. The elif means only the start boundary is trimmed — the end boundary is skipped, so those sessions silently include LFP data beyond end_time. The fix uses two independent if blocks so both boundaries are always evaluated, with file_start parsed only once.

---

D — # REMOVE LATER comment in STTC.make()

Replaced with a proper comment: # clip spike times to session duration to guard against edge-case spikes beyond end_time. The line itself is necessary — without it, neo.SpikeTrain raises when any spike time exceeds t_stop.

---

E — ImpedanceMeasurements.make(): existence check before fetch

Moved if not (EphysSessionProbe & key) before the fetch("port_id") call. Previously, if no EphysSessionProbe existed, the fetch silently returned an empty set and port_id.pop() raised a KeyError instead of the intended ValueError. Also removed the duplicate comment that incorrectly said "EphysSession" instead of "EphysSessionProbe".

---

Test plan

• Verify from element_array_ephys.ephys_no_curation import map_channel_to_electrode, get_probe_type imports without error in an environment without neo/elephant
• Verify map_channel_to_electrode returns correct electrode indices for a known session
• Verify LFP extraction for a session contained within a single .rhd file produces correct duration
• Verify STTC.make() runs end-to-end for a session with spike sorted units

🤖 Generated with Claude Code

[MilagrosMarin]

PR #66 Self-Review — Review fixes for PR #65

This is a self-review of the changes in this PR before requesting @judewerth's sign-off. All 5 originally identified issues are addressed, plus 2 additional edge cases caught during review.

---

✅ Issue A — Lazy imports for neo, quantities, elephant

Confirmed: all three removed from module level, placed inside STTC.make(). Module now imports cleanly in environments without these packages.

---

✅ Issue B — map_channel_to_electrode: restricted fetch + conflict detection + edge cases

Original fix: probe.ElectrodeConfig.Electrode & {"probe_type": probe_type} (restricted), np.unique deduplication, conflict ValueError.

Two additional edge cases fixed in commit 61a37a9:

1. Empty fetch guard — if no ElectrodeConfig has been inserted yet for the probe type, the fetch returns empty arrays and np.column_stack([[], []]) raises a ValueError with a cryptic message. Added an explicit check with a descriptive error before reaching column_stack.

2. np.empty → np.full(..., -1) — np.empty creates an uninitialized array, so any lookup index not covered by the electrode config (e.g., partial configs) would silently return whatever value was in memory. Using -1 as a sentinel makes uncovered positions detectable rather than silently wrong.

One minor suggestion for @judewerth (non-blocking): The return variable electrode_ids and the docstring Returns: electrodes (array-like)... are misleading when electrode_to_channel=True — the function returns channel indices in that direction, not electrode indices. Worth renaming to output_indices or updating the docstring in a follow-up.

---

✅ Issue C — LFP boundary elif → two independent if blocks

Confirmed: single-file sessions now correctly trim both start and end boundaries in a single lfps[:, start_idx:end_idx] slice. file_start parsed once. Multi-file and middle-file cases unaffected.

---

✅ Issue D — # REMOVE LATER in STTC.make()

Replaced with proper comment explaining the spike time clipping is required to keep spike times within t_stop for neo.SpikeTrain. Logic is correct and necessary.

---

✅ Issue E — ImpedanceMeasurements.make() existence check before fetch

Confirmed: if not (EphysSessionProbe & key) now runs before the fetch("port_id") call. A missing EphysSessionProbe now raises a clean ValueError instead of a KeyError on .pop().

---

Notes for @judewerth

• This PR supersedes PR #65 — once this merges, #65 can be closed.
• The workflow repo PR datajoint#211 (integrate branch) imports map_channel_to_electrode and get_probe_type from this module — it should not merge until this PR is merged and the installed element is updated.
• neo, quantities, and elephant are not yet listed as dependencies in setup.py. They should be added before merging so the STTC table is installable by other users.

[MilagrosMarin]

Closing in favor of a PR targeting judewerth:main (PR #65 branch) directly.