allow align_to_coord with non-overlapping traces #585
Conversation
WalkthroughAdd a Changes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dascore/proc/align.py (1)
203-215: Add exception chaining for better error context.The error handling logic is correct, but the re-raised exception should use exception chaining to preserve the original traceback and distinguish it from errors in exception handling.
Apply this diff:
- except ValueError: + except ValueError as err: msg = ( f"Trace shift with align_to_coord results in some traces with no " f"overlaps. This is only possible with mode = 'full' not {mode}." ) - raise ParameterError(msg) + raise ParameterError(msg) from errBased on static analysis hints (Ruff B904).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/proc/align.py(4 hunks)tests/test_proc/test_align.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dascore/proc/align.py (2)
dascore/core/coords.py (1)
get_next_index(795-873)dascore/exceptions.py (1)
ParameterError(26-27)
tests/test_proc/test_align.py (4)
dascore/proc/coords.py (1)
update_coords(228-253)dascore/exceptions.py (1)
ParameterError(26-27)dascore/proc/align.py (1)
align_to_coord(253-391)dascore/proc/basic.py (1)
dropna(438-494)
🪛 Ruff (0.14.5)
dascore/proc/align.py
215-215: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ 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.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: Run benchmarks
🔇 Additional comments (5)
dascore/proc/align.py (3)
192-192: LGTM! Function signature correctly extended.The addition of the
modeparameter enables mode-aware behavior as described in the PR objectives.
274-275: LGTM! Documentation clearly describes the new behavior.The docstring update accurately reflects that mode "full" can handle non-overlapping traces, which is the core objective of this PR.
382-382: LGTM! Mode parameter correctly propagated.The
modeparameter is properly passed to_get_shift_indices, completing the mode-aware implementation.tests/test_proc/test_align.py (2)
116-124: LGTM! Fixture correctly creates non-overlapping scenario.The fixture effectively sets up a test case with shifts [0, 8, 16] on a 10-sample time dimension, ensuring the third trace is shifted beyond the available data range. This provides appropriate test coverage for the new mode='full' behavior.
390-401: LGTM! Comprehensive test coverage for non-overlapping traces.The test correctly validates that:
- Modes "valid" and "same" raise
ParameterErrorfor non-overlapping traces- Mode "full" succeeds even with non-overlapping traces
- The result contains no valid data after dropping NaNs
This aligns perfectly with the PR objectives.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #585 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 129 129
Lines 11233 11237 +4
=======================================
+ Hits 11225 11229 +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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dascore/proc/align.py (1)
192-192: Consider adding type annotation for consistency.The
modeparameter lacks a type annotation. For consistency withalign_to_coord(line 255), consider addingLiteral["full", "valid", "same"]to improve type safety.Apply this diff to add type annotation:
-def _get_shift_indices(coord_vals, dim, reverse, samples, mode): +def _get_shift_indices(coord_vals, dim, reverse, samples, mode: Literal["full", "valid", "same"]):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dascore/proc/align.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dascore/proc/align.py (2)
dascore/core/coords.py (1)
get_next_index(795-873)dascore/exceptions.py (1)
ParameterError(26-27)
⏰ 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 (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: Run benchmarks
🔇 Additional comments (2)
dascore/proc/align.py (2)
203-215: Well-implemented error handling for non-overlapping traces.The try-except block correctly enables out-of-bounds shifts when
mode == "full"and provides clear error messages for other modes. The exception chaining withfrom errpreserves debugging context.
382-382: All call sites correctly updated with themodeparameter.Verification confirms there is only one call site to
_get_shift_indices(line 382), and it correctly passes themodeparameter matching the updated function signature (line 192). No outstanding issues remain.
CodSpeed Performance ReportMerging #585 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dascore/proc/align.py (2)
192-192: Add type hint and update docstring for the newmodeparameter.The function signature now includes a
modeparameter, but it lacks a type hint and the docstring doesn't document what this parameter does. Addingmode: str(ormode: Literal["full", "valid", "same"]) would improve type safety, and updating the docstring would help future maintainers understand the parameter's purpose.Apply this diff to add a type hint:
-def _get_shift_indices(coord_vals, dim, reverse, samples, mode): +def _get_shift_indices(coord_vals, dim, reverse, samples, mode: str):Consider also updating the docstring to document the
modeparameter and its effect onallow_out_of_bounds.
203-215: The error handling logic is correct and aligns with the PR objectives.The try/except block appropriately catches ValueError when traces don't overlap and
allow_out_of_bounds=False, then converts it to a more informative ParameterError. The conditionalallow_out_of_bounds=mode == "full"correctly enables out-of-bounds shifts only for mode='full'.For slightly improved clarity, consider rephrasing the error message:
- msg = ( - f"Trace shift with align_to_coord results in some traces with no " - f"overlaps. This is only possible with mode = 'full' not {mode}." - ) + msg = ( + f"Trace shift with align_to_coord results in some traces with no " + f"overlaps. This is only possible with mode='full', but mode is '{mode}'." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dascore/proc/align.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dascore/proc/align.py (2)
dascore/core/coords.py (1)
get_next_index(795-873)dascore/exceptions.py (1)
ParameterError(26-27)
⏰ 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 (macos-latest, 3.13)
- 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.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.10)
- 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: Run benchmarks
🔇 Additional comments (2)
dascore/proc/align.py (2)
274-275: Docstring accurately reflects the new behavior.The updated documentation correctly describes that mode='full' allows cases where traces do not overlap at all, which aligns with the functional changes introduced in this PR.
382-382: LGTM! Parameter propagation is correct.The
modeparameter is correctly passed from thealign_to_coordfunction to_get_shift_indices, completing the propagation chain needed for the new functionality.
Description
This PR makes it so
align_to_coordcan, when using mode='full' allow for a patch where there is no overlap between all traces.Checklist
I have (if applicable):
Summary by CodeRabbit
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.