Skip to content

feat(extension_types): add LogicalSISortingAnalyzer for SpikeInterface SortingAnalyzer objects#207

Merged
eywalker merged 10 commits into
mainfrom
eywalker/itl-469-support-spikeinterface-sortinganalyzer-objects
Jul 3, 2026
Merged

feat(extension_types): add LogicalSISortingAnalyzer for SpikeInterface SortingAnalyzer objects#207
eywalker merged 10 commits into
mainfrom
eywalker/itl-469-support-spikeinterface-sortinganalyzer-objects

Conversation

@kurodo3

@kurodo3 kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds LogicalSISortingAnalyzer — a new BaseLogicalType that serialises a folder-backed SortingAnalyzer as a pa.large_string() Arrow column containing {"folder": "...", "format": "..."} JSON
  • Adds SISortingAnalyzerHandler — computes a ContentHash via SHA-256 of the folder path string (phase 1; content hashing deferred to ITL-476)
  • Wires both into register_spikeinterface_types(), extension_types/__init__.py, and contexts/data/v0.1.json (with _optional: true so the context degrades gracefully when spikeinterface is not installed)
  • In-memory analyzers (analyzer.folder is None) raise a clear ValueError at both serialisation and hashing time

Test Plan

  • test_logical_si_sorting_analyzer_importable — class is importable and has correct attributes
  • test_in_memory_analyzer_raisespython_to_storage raises ValueError for in-memory analyzers
  • test_binary_folder_analyzer_round_trip — serialise → deserialise round-trip for binary_folder format
  • test_zarr_analyzer_round_trip — serialise → deserialise round-trip for zarr format
  • test_si_sorting_analyzer_handler_hash_stability — same folder → same hash across calls
  • test_si_sorting_analyzer_handler_hash_changes_with_path — different folder → different hash
  • test_si_sorting_analyzer_handler_in_memory_raises — handler raises ValueError for in-memory analyzer
  • test_si_sorting_analyzer_handler_type_error — handler raises TypeError for wrong type
  • test_register_spikeinterface_types_includes_sorting_analyzer — registration wires up the type
  • Full test suite: 4049 passed, 56 skipped, 6 xfailed — zero regressions

Closes ITL-469

🤖 Generated with Claude Code

kurodo3 Bot and others added 8 commits July 3, 2026 04:09
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add LogicalSISortingAnalyzer logical type for spikeinterface.core.SortingAnalyzer.
Stores folder-path JSON (folder + format) instead of to_dict() JSON since
SortingAnalyzer is not a BaseExtractor subclass. Includes full python_to_storage
and storage_to_python implementations and importable test.
…ts (ITL-469)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements SISortingAnalyzerHandler which computes a SHA-256 ContentHash
of the folder path string for SortingAnalyzer objects (phase 1 hashing).
Adds 3 handler tests covering hash stability, path-based differentiation,
and ValueError on in-memory analyzers.
…r (ITL-469)

Add isinstance(obj, SortingAnalyzer) type check to SISortingAnalyzerHandler.handle,
consistent with SIMotionHandler, SIRecordingHandler, and SISortingHandler, so that
passing the wrong type raises a clear TypeError instead of an unguarded AttributeError.
Add test_si_sorting_analyzer_handler_type_error to verify the guard.

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

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

Wire LogicalSISortingAnalyzer and SISortingAnalyzerHandler into the public API
(__init__.py conditional imports + __all__) and into the default context
(v0.1.json logical_types + handlers + changelog). Update module and test
docstrings to include ITL-469.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 3, 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 first-class support for SpikeInterface SortingAnalyzer objects in orcapod’s extension-type system by representing them as a folder-backed reference (JSON stored in an Arrow large_string) and providing a semantic-hash handler keyed off the analyzer folder path.

Changes:

  • Introduces LogicalSISortingAnalyzer (JSON {folder, format} storage in pa.large_string()) and SISortingAnalyzerHandler (SHA-256 of folder path string).
  • Wires the new logical type + handler into SpikeInterface registration, optional exports, and the default context config (v0.1.json).
  • Adds comprehensive pytest coverage for serialization, error cases, hashing stability, and registration.

Reviewed changes

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

Show a summary per file
File Description
tests/test_extension_types/test_spikeinterface_types.py Adds helper + tests for SortingAnalyzer logical type, hashing handler, and registration wiring.
superpowers/specs/2026-07-03-sorting-analyzer-logical-type-design.md Adds a design/spec document for the new logical type and handler.
superpowers/plans/2026-07-03-sorting-analyzer-logical-type.md Adds an implementation plan detailing stepwise changes and test strategy.
src/orcapod/extension_types/spikeinterface_types.py Implements LogicalSISortingAnalyzer, SISortingAnalyzerHandler, and registers them in register_spikeinterface_types().
src/orcapod/extension_types/init.py Exposes the new SpikeInterface logical type/handler behind the optional SpikeInterface import gate.
src/orcapod/contexts/data/v0.1.json Adds optional context entries for the new logical type and handler, plus a changelog entry.

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

Comment on lines +27 to +31
| Logical type name | `"spikeinterface.sorting_analyzer"` |
| Python type | `spikeinterface.core.SortingAnalyzer` |
| Arrow storage type | `pa.large_string()` |
| Arrow extension name | `"orcapod.spikeinterface.sorting_analyzer"` |

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 in commit 934df86. The spec now reads "spikeinterface.sorting_analyzer" to match the implementation and all other SI logical types (recording, sorting, motion).

Comment on lines +60 to +64
### `SISortingAnalyzerHandler.handle(analyzer, hasher)` → `ContentHash`

1. Check `analyzer.folder is None` → raise `ValueError`.
2. Return `ContentHash(hashlib.sha256(str(analyzer.folder).encode()).hexdigest())`.

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 in commit 934df86. The spec pseudocode now reads ContentHash(method="sha256", digest=hashlib.sha256(str(analyzer.folder).encode()).digest()), matching the actual ContentHash(method: str, digest: bytes) dataclass contract.

- Arrow extension name was "orcapod.spikeinterface.sorting_analyzer";
  correct value (matching implementation and all other SI types) is
  "spikeinterface.sorting_analyzer"
- ContentHash pseudocode used .hexdigest() returning str; correct API
  is ContentHash(method="sha256", digest=...) with raw bytes from
  .digest()
@kurodo3

kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Review round addressed (Copilot)

Both comments pointed to inaccuracies in the spec document only — the implementation was already correct.

Changes made (commit 934df86b)

File: superpowers/specs/2026-07-03-sorting-analyzer-logical-type-design.md

Issue Fix
Arrow extension name listed as "orcapod.spikeinterface.sorting_analyzer" Corrected to "spikeinterface.sorting_analyzer" — consistent with the implementation and all other SI logical types
Hashing pseudocode used .hexdigest() (returns str) Updated to ContentHash(method="sha256", digest=hashlib.sha256(...).digest()) — matches the actual ContentHash(method: str, digest: bytes) dataclass

No production code or test changes were required.

…r (ITL-469)

- test_si_sorting_analyzer_storage_to_python_bad_json: exercises the
  except (JSONDecodeError, TypeError) branch in storage_to_python
- test_register_spikeinterface_types_sorting_analyzer_reraises_unexpected_error:
  exercises the re-raise path when register_logical_type raises an
  unexpected ValueError (no "already bound to" in message)

Brings patch coverage for the ITL-469 additions to 100%.
@kurodo3

kurodo3 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Coverage addressed (Codecov)

Codecov reported 3 uncovered lines from the patch in spikeinterface_types.py.

Commit faabf202 adds two tests to tests/test_extension_types/test_spikeinterface_types.py:

Test Lines covered
test_si_sorting_analyzer_storage_to_python_bad_json Lines 725–726 — the except (JSONDecodeError, TypeError) error path in LogicalSISortingAnalyzer.storage_to_python
test_register_spikeinterface_types_sorting_analyzer_reraises_unexpected_error Line 871 — the re-raise path when register_logical_type raises an unexpected ValueError (no "already bound to" in message)

Patch coverage for the ITL-469 additions is now 100%. The remaining uncovered lines in the file are pre-existing gaps in the older SI types (Recording, Sorting, Motion) not part of this PR's patch.

@eywalker eywalker merged commit d6e5da1 into main Jul 3, 2026
11 checks passed
@eywalker eywalker deleted the eywalker/itl-469-support-spikeinterface-sortinganalyzer-objects branch July 3, 2026 05:46
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