Skip to content

feat(extension_types): support SpikeInterface Motion objects (ITL-470)#206

Merged
eywalker merged 9 commits into
mainfrom
eywalker/itl-470-support-spikeinterface-motion-objects
Jul 3, 2026
Merged

feat(extension_types): support SpikeInterface Motion objects (ITL-470)#206
eywalker merged 9 commits into
mainfrom
eywalker/itl-470-support-spikeinterface-motion-objects

Conversation

@kurodo3

@kurodo3 kurodo3 Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add LogicalSIMotion to store spikeinterface.core.motion.Motion as a self-contained NumPy .npz archive in a pa.large_binary() Arrow column — fully portable, no external folder required
  • Add SIMotionHandler computing SHA-256 of the same .npz bytes for content-identity hashing
  • Wire both into register_spikeinterface_types(), v0.1.json (with _optional: true), and extension_types/__init__.py
  • Add 9 new tests covering round-trip, multi-segment, hash stability/sensitivity, TypeError guards, missing-key ValueError, and integration

Closes ITL-470

Test Plan

  • uv run pytest tests/test_extension_types/test_spikeinterface_types.py -v — all 27 tests pass (9 new Motion tests + 18 pre-existing)
  • uv run pytest tests/test_extension_types/ -q — 391 passed, 1 xfailed
  • Single-segment and multi-segment Motion round-trip correctly through python_to_storage / storage_to_python
  • register_spikeinterface_types() is idempotent (safe to call multiple times)
  • When spikeinterface is not installed, all _optional: true JSON entries are silently skipped

🤖 Generated with Claude Code

kurodo3 Bot and others added 7 commits July 2, 2026 21:20
…TL-470)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lType (ITL-470)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds LogicalSIMotion, a BaseLogicalType subclass that serialises
spikeinterface.core.motion.Motion objects as self-contained .npz bytes
in a pa.large_binary() Arrow column. Also adds the module-level helper
_motion_to_npz_bytes() shared by the logical type and the upcoming hash
handler (Task 2). Single- and multi-segment round-trips verified by 3 new tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…(ITL-470)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…register_spikeinterface_types docstring (ITL-470)
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds native orcapod support for serializing and hashing SpikeInterface Motion objects by introducing a new logical type (LogicalSIMotion) backed by a self-contained NumPy .npz payload stored in an Arrow large_binary column, plus a corresponding semantic hash handler (SIMotionHandler) and registration wiring.

Changes:

  • Implement LogicalSIMotion + .npz serialization/deserialization and SIMotionHandler (SHA-256 of the same bytes) in spikeinterface_types.py.
  • Wire Motion logical type + handler into default context registration and v0.1.json optional entries.
  • Add Motion-focused tests covering round-trip, multi-segment behavior, hashing, guards, and integration.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/test_extension_types/test_spikeinterface_types.py Adds unit/integration tests for Motion round-trip and hashing behavior.
superpowers/specs/2026-07-02-spikeinterface-motion-design.md Design spec documenting .npz storage format, hashing, and registration approach.
superpowers/plans/2026-07-02-spikeinterface-motion.md Implementation plan/checklist for adding Motion support.
src/orcapod/extension_types/spikeinterface_types.py Implements Motion logical type + handler and registers them in register_spikeinterface_types().
src/orcapod/extension_types/init.py Exposes SpikeInterface-related symbols conditionally when SpikeInterface is installed.
src/orcapod/contexts/data/v0.1.json Adds optional default-context entries for the Motion logical type and handler plus changelog entry.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +432 to +452
try:
d = np.load(io.BytesIO(bytes(storage_value)), allow_pickle=False)
except Exception as exc:
raise ValueError(
f"LogicalSIMotion: cannot deserialise storage value of type "
f"{type(storage_value)!r}; expected raw .npz bytes."
) from exc
try:
n = int(d["num_segments"][0])
return Motion(
displacement=[d[f"displacement_{i}"] for i in range(n)],
temporal_bins_s=[d[f"temporal_bins_s_{i}"] for i in range(n)],
spatial_bins_um=d["spatial_bins_um"],
direction=str(d["direction"][0]),
interpolation_method=str(d["interpolation_method"][0]),
)
except KeyError as exc:
raise ValueError(
f"LogicalSIMotion: .npz archive is missing expected key {exc}. "
f"The archive may have been produced by a different version of orcapod."
) from exc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I've restructured to open the archive first (so failures get their own error path), then use with npz: to guarantee the NpzFile is closed:

try:
    npz = np.load(io.BytesIO(bytes(storage_value)), allow_pickle=False)
except Exception as exc:
    raise ValueError(f"...: cannot deserialise ...") from exc
with npz:
    try:
        n = int(npz["num_segments"][0])
        return Motion(...)
    except KeyError as exc:
        raise ValueError(f"...: missing expected key ...") from exc

I separated the np.load call from the with block rather than using a single with np.load(...) as d: because both np.load failure and missing-key paths raise ValueError — nesting them inside a single try/except would have made it impossible to distinguish the two error messages cleanly.

Comment thread src/orcapod/extension_types/__init__.py Outdated
Comment on lines +37 to +41
SIRecordingHandler,
LogicalSISorting,
SISortingHandler,
LogicalSIMotion,
SIMotionHandler,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I was overzealous in a previous review round and added SIRecordingHandler/SISortingHandler to the public API when they were never there before. Removed both from the import block. The public surface is now: LogicalSIRecording, LogicalSISorting, LogicalSIMotion, SIMotionHandler (explicitly called out in the feature spec), and register_spikeinterface_types.

Comment on lines +75 to +83
# ITL-459, ITL-468, ITL-470 (conditional — only present when spikeinterface is installed)
*(
[
"LogicalSIRecording", "SIRecordingHandler",
"LogicalSISorting", "SISortingHandler",
"LogicalSIMotion", "SIMotionHandler",
"register_spikeinterface_types",
] if _SI_AVAILABLE else []
),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — removed both SIRecordingHandler and SISortingHandler from all. The export list now matches the original ITL-459/ITL-468 pattern (logical types only) plus SIMotionHandler, which was explicitly specified as public API in the feature design spec for ITL-470.

@kurodo3

kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Review round response

Addressed: Codecov patch coverage gap (92.85% → 95%)

The Codecov report flagged 4 uncovered lines in the new Motion code:

Lines What Fix
434–435 storage_to_python outer except Exception (invalid non-npz bytes) Added test_si_motion_storage_to_python_invalid_bytes
679 register_spikeinterface_types() else branch — first-time registration succeeds Added test_register_spikeinterface_types_motion_fresh_registration (mock context)
674 register_spikeinterface_types() re-raise guard — unexpected ValueError propagates Added test_register_spikeinterface_types_motion_reraises_unexpected_error (mock context with targeted side-effect)

All 3 new tests pass. Full suite: 394 passed, 1 xfailed. Patch coverage now 95%.

The remaining uncovered lines in the file (208–209, 336–337, 524, 577, 636, 655) are pre-existing gaps in the Recording/Sorting code — not part of this PR's patch.

…SIRecording/SortingHandler from public API (ITL-470)
@kurodo3

kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Review round response (Copilot)

Three comments addressed in commit 63424e43:

1. np.load() NpzFile not closed (spikeinterface_types.py:452)

Refactored storage_to_python to use with npz: to properly close the archive. I separated the np.load call from the with block (rather than using a single with np.load(...) as d:) because both np.load failure and missing-key errors raise ValueError — nesting them in one try/except would have conflated the two distinct error messages. The final structure:

try:
    npz = np.load(io.BytesIO(bytes(storage_value)), allow_pickle=False)
except Exception as exc:
    raise ValueError("...: cannot deserialise ...") from exc
with npz:
    try:
        n = int(npz["num_segments"][0])
        return Motion(...)
    except KeyError as exc:
        raise ValueError("...: missing expected key ...") from exc

2 & 3. SIRecordingHandler / SISortingHandler added to public API (__init__.py:41, __init__.py:83)

Removed both from the import block and __all__. They were never part of the public API before this PR and I should not have added them. The exported SI surface is now: LogicalSIRecording, LogicalSISorting, LogicalSIMotion, SIMotionHandler (explicitly specified in the feature design spec), and register_spikeinterface_types.

All 394 tests pass, 1 xfailed.

@eywalker eywalker merged commit c3e0284 into main Jul 3, 2026
11 checks passed
@eywalker eywalker deleted the eywalker/itl-470-support-spikeinterface-motion-objects branch July 3, 2026 03:27
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