Standardize empty-query behavior across interpolators

[ojasvatsyayan]

This PR improves consistency across interpolators for edge cases where no valid times are provided

Changes:
Added warning + empty return handling to SpikeInterpolator
Ensured consistent empty return shape and dtype across interpolators
Preserved existing SequenceInterpolator warning behavior to maintain test compatibility

All existing tests pass locally

Note: SequenceInterpolator and PhaseShiftedSequenceInterpolator warning messages were intentionally left unchanged to preserve compatibility with existing tests

[gitnotebooks]

Review these changes at https://app.gitnotebooks.com/sensorium-competition/experanto/pull/145

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[Copilot]

Pull request overview

This PR aims to standardize how interpolators behave when no valid query times are provided, focusing on consistent warning behavior and consistent empty return outputs across interpolator implementations.

Changes:

• Updated empty-query handling paths in SequenceInterpolator, PhaseShiftedSequenceInterpolator, TimeIntervalInterpolator, and SpikeInterpolator.
• Adjusted warning messaging for empty-query cases to improve consistency.
• Renamed ScreenInterpolator’s internal cache flag from cache_trials to cache_data and updated its usage when constructing trials.

---

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

[Copilot]

Pull request overview

This PR standardizes how interpolators behave when interpolation is requested with no valid times (empty-query), aiming for consistent empty return shapes/dtypes and consistent warning behavior across modalities.

Changes:

• Updated SequenceInterpolator and PhaseShiftedSequenceInterpolator empty-query paths to return empty arrays with an explicit dtype matching the underlying data.
• Standardized the “no valid times” warning message used by TimeIntervalInterpolator and SpikeInterpolator and added the warning to SpikeInterpolator’s empty-query path.
• Renamed ScreenInterpolator’s internal cache flag from cache_trials to cache_data and propagated it into trial creation.

---

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

You can also share your feedback on Copilot code review. Take the survey.

[pollytur]

@ojasvatsyayan could we please merge main into the current branch and also make the CI/CD pass (currently failing at black / isort)

[ojasvatsyayan]

merged latest main, resolved CI issues, ensured formatting + linting pass. ready for review @pollytur @reneburghardt

[pollytur]

@ojasvatsyayan

Note: SequenceInterpolator and PhaseShiftedSequenceInterpolator warning messages were intentionally left unchanged to preserve compatibility with existing tests

what specifically does this mean?

also, if the goal of the PR is to improve consistency - that means you need to make the warnings more consistent and adjust the tests if needed.

plus see the comments to the code changes - in 2 places type should be bool.
Otherwise I see no special value on specifying dtype of the empty array - might remove it for simplicity but no strong opinion

[ojasvatsyayan]

Hi @pollytur ,

Thank you for the feedback. I have now standardized warning message across all interpolators("No valid times provided for interpolation"), ensured all warnings use UserWarning, fixed dtype inconsistencies, removed duplicate warning calls, and updated the test suite accordingly. All tests pass locally, ready for re-review!

Additionally, I’ve been exploring the event-driven data handling in the codebase and am particularly interested in how spike data integrates into the overall pipeline. I started a discussion in the repo and would be happy to continue there, are there any specific directions you'd like me to look into next?

Thanks again for your guidance!

[reneburghardt]

I'll look soonly at the changes, but one additional consistency thing came to my mind @ojasvatsyayan
Could you change all close() implementations of interpolators who are in general able to make use of memmaps to this (as in SpikesInterpolator, adjust variable names of course)?

    def close(self):
        super().close()
        # Trigger cleanup of memmap
        if hasattr(self, "spikes") and isinstance(self.spikes, np.memmap):
            _mmap_obj = getattr(self.spikes, "_mmap", None)
            if _mmap_obj is not None:
                _mmap_obj.close()
            del self.spikes

[ojasvatsyayan]

@pollytur @reneburghardt Thanks for the feedback :) I’ve addressed the requested changes by aligning close() implementations across interpolators with consistent memmap cleanup.

Rebased onto the latest main, resolved conflicts, and all tests pass locally.

Happy to make any further adjustments if needed! Also, if there are other areas of the codebase that could use help or follow-up work, I’d love to take a look.

[ojasvatsyayan]

@reneburghardt @pollytur I standardized the empty-query warning message across the interpolators for consistency, updated the tests accordingly, and also deleted self.trials after ScreenInterpolator cleanup.

[reneburghardt]

LGTM in general, but there are conflicts with the current main which need to be resolved @ojasvatsyayan Can you please do that?