fix align_coords bug #578
fix align_coords bug #578
Conversation
WalkthroughUpdated Changes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #578 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 128 128
Lines 10880 10884 +4
=======================================
+ Hits 10872 10876 +4
Misses 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_proc/test_align.py (1)
113-131: Helper is clear; add a note on units to avoid misuse in samples=True tests.shifts here are absolute-units floats. When used with samples=True later, integer counts are safer to avoid implicit casting/rounding.
Consider documenting this in the docstring or adding an optional dtype parameter (default float) to generate integer shifts when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/proc/align.py(2 hunks)tests/test_proc/test_align.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_proc/test_align.py (4)
dascore/examples.py (1)
get_example_patch(659-706)dascore/proc/coords.py (1)
update_coords(228-253)dascore/proc/align.py (1)
align_to_coord(221-371)dascore/core/patch.py (2)
Patch(28-511)shape(286-299)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: Run benchmarks
🔇 Additional comments (3)
dascore/proc/align.py (2)
94-96: Call site updated correctly.Routing end_shift into _get_dest_indices aligns with the new signature and intended behavior.
41-41: All call sites correctly updated to 5-argument signature.Verification confirms the function signature change at line 41 is consistent with its only call site (lines 94–96). No remaining callers using the old 4-argument form detected.
tests/test_proc/test_align.py (1)
425-445: Good regression coverage for reverse with mode='same'.This reproduces the prior misaligned dest/src sizes and guards against it. Assertions are appropriate.
| def test_reverse_shift_with_larger_shifts(self, random_patch): | ||
| """ | ||
| Test with larger varying shifts to ensure edge cases are handled. | ||
| This creates a more extreme case where the shifts vary significantly, | ||
| which makes the bug more likely to occur. | ||
| """ | ||
| in_patch = random_patch | ||
| patch = _create_patch_with_shifts(start=0, stop=100, patch=in_patch) | ||
| # Align with samples=True, mode="same" | ||
| aligned = patch.align_to_coord( | ||
| relative=False, | ||
| reverse=False, | ||
| samples=True, | ||
| mode="same", | ||
| time="shifts", | ||
| ) | ||
| # Reverse - this should work without broadcasting error | ||
| reversed_patch = aligned.align_to_coord( | ||
| relative=False, | ||
| reverse=False, | ||
| samples=True, | ||
| mode="same", | ||
| time="shifts", | ||
| ) | ||
| assert isinstance(reversed_patch, dc.Patch) |
There was a problem hiding this comment.
Reverse step uses reverse=False; test is not exercising reversal.
The second alignment should set reverse=True to actually reverse the first alignment. Also mirror the shape assertion for stronger validation.
Apply this diff:
- reversed_patch = aligned.align_to_coord(
- relative=False,
- reverse=False,
- samples=True,
- mode="same",
- time="shifts",
- )
+ reversed_patch = aligned.align_to_coord(
+ relative=False,
+ reverse=True,
+ samples=True,
+ mode="same",
+ time="shifts",
+ )
+ # Parity with the first reverse test
+ assert reversed_patch.shape == aligned.shape📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_reverse_shift_with_larger_shifts(self, random_patch): | |
| """ | |
| Test with larger varying shifts to ensure edge cases are handled. | |
| This creates a more extreme case where the shifts vary significantly, | |
| which makes the bug more likely to occur. | |
| """ | |
| in_patch = random_patch | |
| patch = _create_patch_with_shifts(start=0, stop=100, patch=in_patch) | |
| # Align with samples=True, mode="same" | |
| aligned = patch.align_to_coord( | |
| relative=False, | |
| reverse=False, | |
| samples=True, | |
| mode="same", | |
| time="shifts", | |
| ) | |
| # Reverse - this should work without broadcasting error | |
| reversed_patch = aligned.align_to_coord( | |
| relative=False, | |
| reverse=False, | |
| samples=True, | |
| mode="same", | |
| time="shifts", | |
| ) | |
| assert isinstance(reversed_patch, dc.Patch) | |
| def test_reverse_shift_with_larger_shifts(self, random_patch): | |
| """ | |
| Test with larger varying shifts to ensure edge cases are handled. | |
| This creates a more extreme case where the shifts vary significantly, | |
| which makes the bug more likely to occur. | |
| """ | |
| in_patch = random_patch | |
| patch = _create_patch_with_shifts(start=0, stop=100, patch=in_patch) | |
| # Align with samples=True, mode="same" | |
| aligned = patch.align_to_coord( | |
| relative=False, | |
| reverse=False, | |
| samples=True, | |
| mode="same", | |
| time="shifts", | |
| ) | |
| # Reverse - this should work without broadcasting error | |
| reversed_patch = aligned.align_to_coord( | |
| relative=False, | |
| reverse=True, | |
| samples=True, | |
| mode="same", | |
| time="shifts", | |
| ) | |
| # Parity with the first reverse test | |
| assert reversed_patch.shape == aligned.shape | |
| assert isinstance(reversed_patch, dc.Patch) |
🤖 Prompt for AI Agents
In tests/test_proc/test_align.py around lines 446 to 471, the reverse step
mistakenly calls align_to_coord with reverse=False so it doesn't undo the first
alignment; change the second call to reverse=True to actually reverse the first
alignment and add a mirrored shape assertion (e.g., assert reversed_patch.shape
== aligned.shape or assert reversed_patch.samples.shape ==
in_patch.samples.shape) after the reversal to validate the result matches
expected dimensions.
CodSpeed Performance ReportMerging #578 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_proc/test_align.py (1)
446-471: Past review addressed: reverse=True is now correct. Consider adding shape assertion for consistency.The
reverse=Trueparameter on line 466 is now correct (addressing the past review comment). However, for consistency withtest_reverse_shift_with_same_mode(line 444), consider adding a shape assertion after the reversal to strengthen validation.Apply this diff to add the shape assertion:
assert isinstance(reversed_patch, dc.Patch) + assert reversed_patch.shape == aligned.shape
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/proc/align.py(2 hunks)tests/test_proc/test_align.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_proc/test_align.py (4)
dascore/examples.py (1)
get_example_patch(659-706)dascore/proc/coords.py (1)
update_coords(228-253)dascore/proc/align.py (1)
align_to_coord(221-371)dascore/core/patch.py (2)
Patch(28-511)shape(286-299)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: Run benchmarks
🔇 Additional comments (4)
dascore/proc/align.py (2)
41-53: LGTM! Bug fix correctly implements robust per-trace clamping.The updated
_get_dest_indicesfunction now properly computes per-trace source data availability and clamps all indices to valid ranges. This prevents the broadcast error that occurred when source and destination lengths misaligned with varying shifts in mode="same". The implementation follows the defensive clamping pattern suggested in the past review.
94-96: Call site correctly updated with end_shift parameter.The function call properly passes the new
end_shiftparameter required by the updated signature.tests/test_proc/test_align.py (2)
113-131: Helper function is well-designed for test reusability.The
_create_patch_with_shiftsutility cleanly generates patches with linearly varying shifts across the distance dimension, facilitating multiple test scenarios.
425-444: Test effectively verifies the bug fix for mode="same" reverse alignment.This test reproduces the original bug scenario (varying shifts causing broadcast errors during reverse alignment) and confirms the fix works. The shape assertion on line 444 provides good validation of the round-trip behavior.
Description
This PR fixes a bug in
Patch.align_to_coordwhere the source and destination lengths can be misaligned. Since this isn't in a release yet its probably not a big deal.Checklist
I have (if applicable):
Summary by CodeRabbit
Bug Fixes
Tests