fix: linear-interpolation#161
Conversation
|
Review these changes at https://app.gitnotebooks.com/sensorium-competition/experanto/pull/161 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Fixes linear interpolation time calculations when start_time != 0 for both SequenceInterpolator and PhaseShiftedSequenceInterpolator, and extends tests to cover non-zero start times (issue #160).
Changes:
- Add
start_timewhen computingtimes_lower/times_upperinSequenceInterpolator.interpolate(linear mode). - Add
start_timeand per-signal phase shifts totimes_lower/times_upperinPhaseShiftedSequenceInterpolator.interpolate(linear mode), and adjust numerator computation accordingly. - Expand linear interpolation tests to run with
start_timeset to0.0and15.0.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
experanto/interpolators.py |
Corrects linear interpolation time math to account for non-zero start_time (and phase shifts for the phase-shifted interpolator). |
tests/test_sequence_interpolator.py |
Adds coverage for start_time != 0 in linear interpolation tests. |
diff_output.txt |
Newly added artifact file containing an encoded diff; appears unintended. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…9/experanto into fix/linear-interpolation
|
hey @pollytur @schewskone did the changes, happy to get a review :) |
There was a problem hiding this comment.
Pull request overview
Fixes linear interpolation correctness when start_time != 0 for sequence-based interpolators, aligning computed sample times with the underlying timestamp origin (and per-signal phase shifts).
Changes:
- Correct
SequenceInterpolator.interpolate(..., interpolation_mode="linear")to includestart_timeintimes_lower/times_upper. - Correct
PhaseShiftedSequenceInterpolator.interpolate(..., interpolation_mode="linear")to includestart_time(and phase shifts) intimes_lower/times_upper, and apply the overflow row mask consistently to all derived arrays. - Extend linear interpolation tests to cover non-zero
start_time.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
experanto/interpolators.py |
Fixes linear interpolation time math for non-zero start_time (and phase-shifted linear path), ensuring correct weight computation and consistent masking. |
tests/test_sequence_interpolator.py |
Expands linear interpolation test parametrization to validate behavior when start_time is non-zero. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
schewskone
left a comment
schewskone
left a comment
There was a problem hiding this comment.
LGTM, thanks for fixing this.
| times_lower = ( | ||
| self.start_time | ||
| + (idx_lower * self.time_delta) | ||
| + self._phase_shifts[np.newaxis, :] |
There was a problem hiding this comment.
@binary69 why are there self._phase_shifts[ here now while they were not there before
There was a problem hiding this comment.
I just moved it there because the earlier version was just doing the same calculation in additional lines. Changed it since it'll make the code more readable
| times_upper = ( | ||
| self.start_time | ||
| + (idx_upper * self.time_delta) | ||
| + self._phase_shifts[np.newaxis, :] |
There was a problem hiding this comment.
same here - @binary69 why are there self._phase_shifts[ here now while they were not there before
Closes #160
Fixes the linear interpolation in
PhaseShiftedSequenceInterpolator.interpolateandSequenceInterpolator.interpolate