Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 20, 2025

The intervals dimension caused memory waste from zero-padding and confused users with its 3D structure. Decoding results now concatenate along time with an interval_labels coordinate for grouping.

Changes

  • Core logic (sorted_spikes.py, clusterless.py): Changed xr.concat(results, dim="intervals") to dim="time" with interval_labels=("time", [0,0,...,1,1,...]) coordinate
  • Tests: Updated mock decoders and added tests validating no intervals dimension and proper interval_labels tracking

Impact

Memory: 12-48% reduction by eliminating padding zeros (varies with interval length distribution)

Structure:

  • Before: (n_intervals, max_time, n_position) with padding
  • After: (total_time, n_position) without padding

Usage:

results = table.fetch_results()

# Select specific interval
interval_0 = results.sel(time=results.interval_labels == 0)

# Iterate intervals
for idx, data in results.groupby('interval_labels'):
    process(data)
Original prompt

This section details on the original issue you should resolve

<issue_title>Get rid of intervals dimension in decoding.</issue_title>
<issue_description>The intervals dimension often confuses people and can take up more memory because we have to pad to the longest time series with zeros.

We should store it as one time series. There can be an additional variable that tracks the intervals so that people can do a groupby operation or selection to get the interval.</issue_description>

<agent_instructions>Use TDD</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@edeno edeno requested a review from Copilot November 20, 2025 20:30
Copilot finished reviewing on behalf of edeno November 20, 2025 20:32
Copilot AI changed the title [WIP] Remove intervals dimension in decoding process Remove intervals dimension from decoding results Nov 20, 2025
Copilot AI requested a review from edeno November 20, 2025 20:32
Copilot finished work on behalf of edeno November 20, 2025 20:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the intervals dimension from decoding results and replaces it with concatenation along the time dimension, adding an interval_labels coordinate to track interval membership. This change eliminates memory waste from zero-padding shorter time series and reduces confusion for users.

Key changes:

  • Concatenate decoding results along time instead of intervals dimension
  • Add interval_labels coordinate for tracking which interval each time point belongs to
  • Update tests to verify the new structure and demonstrate 12.4% memory savings

Reviewed Changes

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

Show a summary per file
File Description
src/spyglass/decoding/v1/sorted_spikes.py Modified to concatenate results along time dimension and add interval_labels tracking
src/spyglass/decoding/v1/clusterless.py Modified to concatenate results along time dimension and add interval_labels tracking
tests/decoding/conftest.py Updated mock decoder fixtures to simulate the new concatenation behavior
tests/decoding/test_intervals_removal.py Integration tests verifying the new structure with actual decoder tables
tests/decoding/test_intervals_removal_simple.py Standalone unit tests demonstrating memory savings and basic functionality

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

Comment on lines 826 to 829
if len(results_list) == 1:
results = results_list[0]
else:
results = xr.concat(results_list, dim="time")
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The mock decoder handles the single-interval case specially but doesn't handle the case where results_list is empty. Add a check: if not results_list: raise ValueError(\"No intervals to decode\") before the length check.

Copilot uses AI. Check for mistakes.
Comment on lines 1010 to 1013
if len(results_list) == 1:
results = results_list[0]
else:
results = xr.concat(results_list, dim="time")
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The mock decoder handles the single-interval case specially but doesn't handle the case where results_list is empty. Add a check: if not results_list: raise ValueError(\"No intervals to decode\") before the length check.

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

❌ Patch coverage is 44.44444% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.09%. Comparing base (32b6950) to head (281d949).

Files with missing lines Patch % Lines
src/spyglass/decoding/v1/sorted_spikes.py 0.00% 9 Missing ⚠️
src/spyglass/decoding/v1/clusterless.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1476      +/-   ##
==========================================
- Coverage   71.12%   71.09%   -0.03%     
==========================================
  Files         114      114              
  Lines       13306    13318      +12     
==========================================
+ Hits         9464     9469       +5     
- Misses       3842     3849       +7     
Flag Coverage Δ
full-tests 71.09% <44.44%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

edeno and others added 6 commits November 21, 2025 07:06
Introduces an interval_labels coordinate to the results in both ClusterlessDecodingV1 and SortedSpikesDecodingV1 for consistency with the predict branch. This uses scipy.ndimage.label to identify intervals, ensuring that results outside intervals are marked as -1 and intervals are 0-indexed.
Expanded mock decoders and tests to cover both estimate_decoding_params=True and False branches. Tests now verify interval_labels behavior for all time points, including handling of -1 for points outside intervals and groupby/filtering operations. Mock decoders updated to match real logic for interval_labels assignment.
The tests for interval_labels with estimate_decoding_params=True expected
-1 labels for time points outside the decoding interval, but the fixture
created decode_interval covering the same range as the position data.
Changed decode_interval from [raw_begin, raw_begin+15] to
[raw_begin+2, raw_begin+13] so position_info has time points outside
the decoding interval, which correctly get interval_labels=-1.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The mock decoding results had 'states' and 'state_ind' coordinates that
could conflict with the 'state' dimension, causing xarray broadcasting
errors during groupby operations. Removed these redundant coordinates.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Changed dimension name from 'state' to 'states' to match the actual
non_local_detector output. Also restored state_ind coordinate which
is part of the real detector output. This fixes the xarray broadcasting
error in groupby operations.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
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.

Get rid of intervals dimension in decoding.

2 participants