Fix align_to_coord and plot #582
Conversation
WalkthroughRemoved legacy relative-based alignment behavior and switched alignment functions to operate on explicit coordinate values; added coordinate-value validation helper, simplified shift-index/aligned-coords logic, normalized shift_time in fixtures/benchmarks, adjusted viz/mute docs, and reorganized/added alignment tests including new validation cases. 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dascore/proc/align.py (1)
179-189: LGTM - Simplified function signature.The
_get_aligned_coordsfunction has been simplified by removing theshift_coord,samples, andrelativeparameters. The function now derives the new coordinate bounds purely from the shift metadata (start_shift,end_shift), which is cleaner and more maintainable.Consider adding a docstring to explain how start_shift and end_shift are interpreted:
def _get_aligned_coords(patch, dim_name, meta): """ Get the aligned coordinate manager. + + Parameters + ---------- + patch : Patch + The input patch. + dim_name : str + The dimension being aligned. + meta : _ShiftInfo + Shift metadata containing start_shift and end_shift which define + the new coordinate bounds in sample space. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
benchmarks/test_align_benchmarks.py(0 hunks)benchmarks/test_patch_benchmarks.py(1 hunks)dascore/proc/align.py(7 hunks)dascore/proc/mute.py(2 hunks)dascore/viz/waterfall.py(2 hunks)tests/test_core/test_attrs.py(1 hunks)tests/test_core/test_coords.py(1 hunks)tests/test_integrations/test_misc_integrations.py(2 hunks)tests/test_proc/test_align.py(8 hunks)
💤 Files with no reviewable changes (1)
- benchmarks/test_align_benchmarks.py
🧰 Additional context used
🧬 Code graph analysis (5)
benchmarks/test_patch_benchmarks.py (2)
dascore/proc/coords.py (1)
update_coords(228-253)dascore/proc/align.py (1)
align_to_coord(246-378)
dascore/proc/align.py (2)
dascore/core/coords.py (7)
get_compatible_values(170-188)get_coord(1533-1724)min(438-440)get_next_index(795-873)values(1000-1004)values(1259-1274)index(702-721)dascore/utils/time.py (1)
dtype_time_like(419-429)
tests/test_integrations/test_misc_integrations.py (5)
dascore/examples.py (1)
get_example_patch(659-706)dascore/proc/coords.py (1)
update_coords(228-253)dascore/proc/correlate.py (1)
correlate(88-225)dascore/proc/align.py (1)
align_to_coord(246-378)dascore/proc/basic.py (1)
dropna(438-494)
dascore/viz/waterfall.py (2)
dascore/utils/time.py (2)
dtype_time_like(419-429)is_datetime64(409-411)dascore/utils/plotting.py (1)
_format_time_axis(84-103)
tests/test_proc/test_align.py (4)
dascore/utils/time.py (1)
to_datetime64(22-55)dascore/proc/coords.py (1)
update_coords(228-253)dascore/exceptions.py (1)
ParameterError(26-27)dascore/proc/align.py (1)
align_to_coord(246-378)
⏰ 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 (macos-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: Run benchmarks
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- 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.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
🔇 Additional comments (22)
dascore/proc/mute.py (2)
544-544: LGTM - Documentation improvement.The reference has been corrected to point to the more specific
Patch.mute.line_muteinstead of the genericPatch.mute.
596-596: LGTM - Consistent documentation update.This matches the documentation correction on line 544, ensuring consistency across the docstrings.
tests/test_core/test_coords.py (1)
503-507: LGTM - Test reorganization.The
test_select_end_end_timetest has been moved but its logic remains unchanged. This appears to be a minor organizational improvement.tests/test_core/test_attrs.py (1)
215-215: LGTM - Typo fix.The method name has been corrected from
test_can_roundriptotest_can_roundtrip.dascore/viz/waterfall.py (2)
22-22: LGTM - Import added for refined time-type checking.The
is_datetime64import enables more precise differentiation between datetime64 and timedelta64 types in axis formatting logic.
116-123: LGTM - Improved axis formatting logic.This change correctly separates two concerns:
- Time-axis formatting (line 118): Now uses
is_datetime64to apply date formatting only to datetime64 axes, not timedelta64 axes- Y-axis inversion (line 122): Still uses
dtype_time_liketo invert both datetime64 and timedelta64 axesThis addresses the PR objective of preventing timedelta64 y-axes from being incorrectly formatted as datetime64 axes.
benchmarks/test_patch_benchmarks.py (3)
340-343: LGTM - Coordinate naming updated for new alignment API.The benchmark now computes relative shifts explicitly (
shift_relative = shift_time_absolute - shift_time_absolute.min()) and assigns them as theshift_timecoordinate. This aligns with the refactoredalign_to_coordAPI where coordinate values drive the shift behavior rather than a separaterelativeparameter.
349-349: LGTM - Benchmark updated to use new coordinate name.The test now references
shift_timeinstead ofshift_time_absolute, consistent with the fixture changes above.
355-355: LGTM - Consistent benchmark update.Matches the coordinate naming update on line 349.
tests/test_integrations/test_misc_integrations.py (2)
5-5: LGTM - Import added for test assertions.The numpy import is used for
np.allcloseassertion in the new correlation test.
50-95: LGTM - Comprehensive integration test for alignment and correlation.This new test class validates an important property: that shifting can be reversed correctly by performing forward alignment before correlation and reverse alignment after. The test:
- Creates a ricker patch with known velocity-based shifts
- Compares correlation without shifts vs. correlation with forward+reverse alignment
- Verifies data equivalence using
np.allcloseThis provides valuable regression protection for the alignment refactoring.
dascore/proc/align.py (6)
14-14: LGTM - Import added for coordinate value validation.The
get_compatible_valuesfunction is used in the new_get_coord_valueshelper to ensure dtype compatibility between dimensions and shift coordinates.
18-18: LGTM - Import added for time-type validation.The
dtype_time_likefunction is used in_get_coord_valuesto handle datetime/timedelta coordinate validation.
165-165: LGTM - Improved error message.The error message now shows
tuple(patch.coords.coord_map)which provides a clearer view of available coordinates.
214-242: LGTM - Well-designed validation helper.The new
_get_coord_valuesfunction provides thorough validation of coordinate values:
- Samples mode validation (lines 220-226): Ensures coordinates are integer-type when using
samples=True- Dtype compatibility validation (lines 229-242): Converts coordinate values to be compatible with the dimension's dtype, with special handling for time-like types
The error messages are clear and helpful. The use of
get_compatible_valuesfor datetime conversions is appropriate.
245-378: Breaking change properly implemented and all usages updated.Verification confirms the
relativeparameter removal has been consistently applied:
- Zero residual usages of old
relativeparameter anywhere in codebase (test files, integrations, benchmarks all updated)- Master-only feature:
align_to_coordintroduced in commit d1cc5a9; no released version tags contain this commit- All callsites migrated: Tests, integrations, and benchmarks use new API with
samples,mode, andreverseparameters- API consistency: Docstring examples match implemented signature
The breaking change is acceptable and properly scoped.
192-212: Shift index calculation logic is correct — no changes needed.The refactored
_get_shift_indicesfunction correctly decomposes coordinate values into magnitude and sign, applies the reverse modifier appropriately, and the logic aligns with the docstring. Tests validate the behavior through magnitude checking (_test_mode_sameassertsnan_count == shift) and round-trip validation (forward + reverse alignment approximately restores data). Bothsamples=Trueand coordinate-value modes are tested and functioning as expected.tests/test_proc/test_align.py (5)
24-30: LGTM - Test fixture updated for new coordinate naming.The fixture now creates a
bad_type_shiftcoordinate usingdc.to_datetime64(distance)instead of the previousbad_type_coord. This aligns with the new validation logic that checks dtype compatibility between shift coordinates and the target dimension.
57-62: LGTM - 4D fixture updated with new coordinate naming.The shift coordinates have been renamed from
shift_time_absolute_1d/2d/3dtoshift_time_1d/2d/3d, and the values are now explicitly made relative by subtracting the minimum (e.g.,shift_time_2d - shift_time_2d.min()). This aligns with the new API where coordinate values drive the shift behavior.
94-94: LGTM - Explicit dtype for zero shifts.Adding
dtype=np.int64to thezero_shiftsarray ensures consistency with the samples mode requirements where integer types are expected.
171-183: LGTM - New validation tests added.Two new test methods have been added:
test_samples_non_int_raises(lines 171-176): Validates that usingsamples=Truewith non-integer coordinates raises an appropriate errortest_bad_alignment_dim(lines 178-183): Validates that incompatible dtypes between shift coordinate and target dimension raise an errorThese tests provide good coverage for the new validation logic in
_get_coord_values.
337-358: LGTM - Updated round-trip test for new API.The
test_align_round_triptest has been updated to work with the new API:
- Creates relative shifts explicitly by subtracting the minimum (line 344)
- Uses the coordinate name
rel_shiftsinstead of relying on arelativeparameter (lines 342, 348, 352)- Validates that forward and reverse alignment with
mode="full"produces a round-trip (line 357)This test effectively validates the core functionality of the refactored alignment system.
CodSpeed Performance ReportMerging #582 will not alter performanceComparing Summary
Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #582 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 129 129
Lines 11207 11225 +18
=======================================
+ Hits 11199 11217 +18
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dascore/proc/align.py (1)
255-333: Docstring still references removedrelativeparameter and old semanticsThe
align_to_coorddocstring has been updated, but a few bits are now inconsistent with the implementation:
Under
samples:“If False and relative is False (absolute mode) ...”
There is no
relativeparameter anymore and the implementation only supports relative shifts. This sentence is misleading and should be rewritten to describe the current behavior (samples=Falsealways interprets values as relative offsets in the dimension’s units).The initial description:
“The specified coordinate must indicate a shift relative to the current positions.”
This matches the new behavior and is good; it just conflicts with the leftover mention of “absolute mode”.
Example variable naming (
aligned_abs) still suggests an “absolute” alignment mode, even though only relative shifts are now supported. Renaming the variable (e.g.,aligned_full) would reduce confusion.Recommend updating the parameter doc and examples so that they no longer mention absolute mode or a
relativeflag and clearly describe that all non-sample shifts are relative to the dimension.
🧹 Nitpick comments (3)
dascore/proc/align.py (3)
179-189: Check assumptions aroundcoord.stepin_get_aligned_coords
_get_aligned_coordsnow computesnew_min = coord.min() + coord.step * start_shift new_coord = coord.update(min=new_min).change_length(len(coord) - start_shift + end_shift)This assumes
coord.stepis meaningful and non-Nonefor the aligned dimension in all modes. However,align_to_coordonly enforcesrequire_evenly_sampled=Truewhenmode == "full"; for"same"/"valid"an uneven or array-based coordinate may be returned, wherecoord.stepmay not represent a fixed sampling interval.If uneven coordinates are allowed for
"same"/"valid"today, this change can break them or subtly misplace the new min.Two possible directions:
- Enforce
require_evenly_sampled=Truefor all modes, or- Make
_get_aligned_coordsrobust to non-uniform coords (e.g., derive the new coordinate from existingcoord.valuesrather thanmin + step * ...).Please double-check existing usage/tests for non-evenly-sampled dims and adjust either the requirement or this helper accordingly.
336-341: Note section wording can drop legacy “relative” terminologyIn the notes:
“alignment dimension length of 10, and a relative shift of 5 samples.”
Given the API now only supports relative shifts, this can be phrased more simply as “a shift of 5 samples” without referencing relative vs absolute modes. Not a correctness issue, just a small clarity improvement to fully align with the new API.
360-361: Validation call into_get_coord_valuesis a good centralization (typo nit)Routing all dtype and “samples vs. values” checking through
_get_coord_valuesprior to computing shift indices is a solid simplification and will make the behavior easier to reason about.Very minor nit: the comment reads
# Ensure valide dim and coordinate.; consider fixing the typo (valid) when you next touch this area.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dascore/proc/align.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dascore/proc/align.py (2)
dascore/core/coords.py (7)
get_compatible_values(170-188)get_coord(1533-1724)min(438-440)get_next_index(795-873)values(1000-1004)values(1259-1274)index(702-721)dascore/utils/time.py (1)
dtype_time_like(419-429)
⏰ 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). (19)
- GitHub Check: test_build_docs
- 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 (windows-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- 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 (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: Run benchmarks
🔇 Additional comments (4)
dascore/proc/align.py (4)
14-19: New imports for coord/value handling look appropriateBringing in
get_compatible_valuesanddtype_time_likeis consistent with the new_get_coord_valueshelper and centralizes dtype coercion logic instead of rolling custom conversions here. No issues with these imports.
160-167: Improved error message for invalid coord nameUsing
tuple(patch.coords.coord_map)produces a clearer, more readable list of available coordinate names in the error, which should help users debug mis-specifiedalign_to_coordcalls.
214-242: ---Consider using
np.can_castinstead ofnp.issubdtypefor dtype compatibility validationThe dtype validation logic is sound and prevents misuse, but the tool choice can be clarified. NumPy best practices recommend:
np.issubdtype: for type hierarchy checks only (e.g., "is integer" or "is floating"); does NOT imply safe/value-preserving casts.np.can_cast: for validating dtype conversion compatibility with explicit casting semantics (safe, same_kind, etc.).The current line at 236:
if not np.issubdtype(expected_dtype, coord_values.dtype):Is a hierarchy check rather than a casting validation. If the intent is to ensure coordinate values can be safely used in alignment, consider:
if not np.can_cast(coord_values.dtype, expected_dtype, casting="safe"):This more explicitly expresses the intent and aligns with NumPy conventions for dtype compatibility checks.
367-378: Code review verification complete — no fixes requiredThe code at lines 367-378 is correct and well-tested. Verification confirms:
Time-like coordinate support:
test_shift_2d_relativedirectly exercises the code path with timedelta64 coordinates andsamples=False, confirming alignment works with relative time shifts across all modes ("full", "same", "valid").Round-trip alignment with
reverse=True:test_round_trip_with_reverse(line 299) verifies that forward and reverse alignments work correctly, including the specific scenario mentioned in the docstring example (lines 312-325).Sign handling is valid:
np.sign()correctly handles timedelta64 values in NumPy, preserving the sign while converting to indices. The multiplication at line 210 (sign_vals * (-1 if reverse else 1)) properly inverts shifts whenreverse=True.The refactored pipeline is clearer and behaves as documented. All concerns raised in the original review have been addressed by the current implementation.
|
✅ Documentation built: |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
dascore/proc/align.py (1)
192-210: Critical bug:np.signon timedelta64 still produces incorrect shift indices.The issue flagged in the previous review remains: when
coord_valsis timedelta64,np.sign(coord_vals)returns timedelta64 values (not integers -1, 0, 1). Although line 209 adds.astype(np.int64), this interprets the timedelta as nanoseconds, producing shifts with completely wrong magnitudes. This breaks alignment for timedelta64 coordinates—a primary use case shown in the docstring at line 306.Apply this fix to compute sign explicitly as integers:
def _get_shift_indices(coord_vals, dim, reverse, samples): """ Get the indices for shifting. Positive values indicate a shift to the right, negative to left. """ - # Relative means something slightly different here. It only means - # move backwards, rather than implying a reference from the end of the trace. - # Therefore, we need to insure positive values go into get_next_index. abs_vals = np.abs(coord_vals) - sign_vals = np.sign(coord_vals) + # Compute sign as integers so it works for both numeric and time-like dtypes. + sign_vals = np.zeros_like(abs_vals, dtype=np.int64) + sign_vals[coord_vals > 0] = 1 + sign_vals[coord_vals < 0] = -1 inds_abs = dim.get_next_index( abs_vals, samples=samples, relative=False if samples else True, ) - # Reverse index if needed. This way the reference stays the same. - inds = inds_abs * (sign_vals.astype(np.int64) * (-1 if reverse else 1)) + factor = -1 if reverse else 1 + inds = inds_abs * (sign_vals * factor) return indsBased on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/proc/align.py(7 hunks)dascore/viz/waterfall.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dascore/viz/waterfall.py (2)
dascore/utils/time.py (2)
dtype_time_like(419-429)is_datetime64(409-411)dascore/utils/plotting.py (1)
_format_time_axis(84-103)
dascore/proc/align.py (3)
dascore/core/patch.py (2)
coords(255-269)dtype(318-320)dascore/core/coords.py (7)
get_compatible_values(170-188)get_coord(1533-1724)min(438-440)get_next_index(795-873)values(1000-1004)values(1259-1274)index(702-721)dascore/utils/time.py (1)
dtype_time_like(419-429)
⏰ 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). (19)
- 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 (macos-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 (windows-latest, 3.13)
- GitHub Check: test_build_docs
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-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.10)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: Run benchmarks
🔇 Additional comments (9)
dascore/viz/waterfall.py (4)
8-8: LGTM: Imports are appropriate.The new imports support the datetime handling improvements and rcParams configuration.
Also applies to: 23-23
123-130: LGTM: Datetime/timedelta distinction correctly implemented.The use of
is_datetime64for formatting (line 125) anddtype_time_likefor y-axis inversion (line 129) is intentional. This ensures that only datetime64 axes receive datetime formatting, while both datetime64 and timedelta64 axes are inverted. This aligns with the PR objective to prevent timedelta64 axes from being formatted as datetime64 axes.
269-269: LGTM: Interpolation parameter correctly passed to imshow.The interpolation parameter is properly threaded through to matplotlib's imshow, giving users control over downsampling behavior for large arrays.
272-273: LGTM: Defensive color scale handling with robustness improvements.The updated guard adds an
np.isfinitecheck to prevent setting invalid color limits (NaN or inf). Thenp.asarraywrapper is defensive but harmless since_get_scalealready returns numpy arrays. Thescale is not Nonecheck appears unnecessary based on_get_scalealways returning an array, but adds defensive robustness.dascore/proc/align.py (5)
14-14: LGTM! Appropriate imports for new validation logic.The new imports
get_compatible_valuesanddtype_time_likesupport the enhanced dtype validation and coordinate value handling introduced in this refactor.Also applies to: 18-18
165-165: LGTM! Improved error message clarity.Explicit tuple conversion ensures consistent formatting in the error message.
179-189: LGTM! Simplified and correct coordinate alignment logic.The direct computation
new_min = coord.min() + coord.step * start_shiftworks correctly for both numeric and time-like coordinates, and the simplified signature improves maintainability.
213-241: LGTM! Well-structured validation logic.The new
_get_coord_valueshelper properly validates dtype compatibility between the dimension and coordinate, with clear error messages for incompatible cases. The handling of time-like dtypes and samples mode is correct.
245-377: Simplified API and improved clarity.The removal of the
relativeparameter and the introduction of explicit validation via_get_coord_valuesmakes the alignment logic clearer and more maintainable. The updated docstring examples effectively illustrate the new coordinate-based shifting semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dascore/proc/align.py (1)
254-283: Update docstring to match new “relative-only” semantics (remove stalerelativereferences).The
align_to_coorddocstring still references the oldrelativeparameter and “absolute mode”:
- The
samplesdescription mentions “If False and relative is False (absolute mode)…”, butrelativeis no longer a parameter and absolute-position alignment is no longer supported.- The “Mode Parameter” notes talk about “a relative shift of 5 samples” without clarifying that all non-sample alignments are now interpreted as relative offsets.
Please rewrite these sections so they describe the current behavior: coordinate values are always interpreted as relative shifts (or sample offsets when
samples=True), with no absolute-position mode exposed in the API.Also applies to: 335-350
♻️ Duplicate comments (2)
dascore/viz/waterfall.py (1)
176-180: Docstring still has a typo (“relavent”).The interpolation parameter docstring still reads “relavent for DAS”; this should be “relevant for DAS” to fully address the earlier doc typo comment.
dascore/proc/align.py (1)
192-210: Critical:np.signon timedelta64 still produces invalid shift indices.For time-like alignment coordinates (
timedelta64),np.sign(coord_vals)returnstimedelta64, soinds_abs * sign_valsyields timedelta-valued shifts that are later coerced toint64in_calculate_shift_info, effectively scaling shifts by the time unit (e.g., nanoseconds). This matches the earlier critical issue and will badly break time-based alignments.Replace
np.signwith an explicit integer sign array so shifts stay integer for both numeric and time-like dtypes:-def _get_shift_indices(coord_vals, dim, reverse, samples): +def _get_shift_indices(coord_vals, dim, reverse, samples): @@ - abs_vals = np.abs(coord_vals) - sign_vals = np.sign(coord_vals) + abs_vals = np.abs(coord_vals) + # Compute sign as integers so it works for both numeric and time-like dtypes. + sign_vals = np.zeros_like(abs_vals, dtype=np.int64) + sign_vals[coord_vals > 0] = 1 + sign_vals[coord_vals < 0] = -1 @@ - inds_abs = dim.get_next_index( + inds_abs = dim.get_next_index( abs_vals, samples=samples, relative=False if samples else True, ) - # Reverse index if needed. This way the reference stays the same. - inds = inds_abs * (sign_vals.astype(np.int64) * (-1 if reverse else 1)) + # Reverse index if needed. This way the reference stays the same. + factor = -1 if reverse else 1 + inds = inds_abs * (sign_vals * factor) return inds#!/bin/bash # Minimal check: search for uses of np.sign in align.py to ensure all shift-sign paths are updated. rg -n "np\.sign" dascore/proc/align.py -n -C2
🧹 Nitpick comments (2)
dascore/proc/align.py (2)
179-189: Guard use ofcoord.stepwhen mode ≠ "full".
_get_aligned_coordsalways usescoord.stepto computenew_min, butalign_to_coordonly enforcesrequire_evenly_sampled=Truewhenmode == "full". For"same"and"valid"modes,coord.stepmay beNoneor not well-defined for irregular coordinates, which would cause failures or misleading coordinate updates.Consider either:
- requiring
require_evenly_sampled=Truefor all modes ifcoord.stepis relied on, or- handling the
coord.step is None/ irregular case explicitly (e.g., falling back to using the existing coord without recomputingmin).Based on learnings.
Also applies to: 352-357
213-241: Relax dtype compatibility check usingnp.can_castin_get_coord_values.The check at line 237 (
np.issubdtype(expected_dtype, coord_values.dtype)) is overly strict and backwards. NumPy'snp.can_castis the recommended function to check whether values of one dtype can be safely cast to another, whilenp.issubdtypeonly checks type hierarchy and does not consider casting compatibility. This causes false rejections of safe conversions (e.g., int32 → float64).Replace with:
if not np.can_cast(coord_values.dtype, expected_dtype, casting="safe"): ...This maintains validation while accepting safely-castable numeric types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/proc/align.py(6 hunks)dascore/viz/waterfall.py(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T16:22:22.879Z
Learnt from: d-chambers
Repo: DASDAE/dascore PR: 523
File: dascore/transform/fourier.py:433-434
Timestamp: 2025-09-02T16:22:22.879Z
Learning: In dascore STFT implementation, use `require_evenly_sampled=True` on `get_coord` calls instead of manual validation for coord.step to ensure sampling rate calculations are safe.
Applied to files:
dascore/proc/align.py
🧬 Code graph analysis (2)
dascore/proc/align.py (3)
dascore/core/patch.py (4)
coords(255-269)dtype(318-320)shape(286-299)data(272-283)dascore/core/coords.py (8)
get_compatible_values(170-188)get_coord(1533-1724)min(438-440)get_next_index(795-873)values(1000-1004)values(1259-1274)index(702-721)data(606-608)dascore/utils/time.py (1)
dtype_time_like(419-429)
dascore/viz/waterfall.py (2)
dascore/utils/time.py (2)
dtype_time_like(419-429)is_datetime64(409-411)dascore/utils/plotting.py (1)
_format_time_axis(84-103)
⏰ 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). (19)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- 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 (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_build_docs
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: Run benchmarks
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (windows-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.10)
- 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 (ubuntu-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.12)
🔇 Additional comments (2)
dascore/viz/waterfall.py (1)
111-124: Axis time-handling and rc_context usage look correct.Using
is_datetime64for formatter setup while still usingdtype_time_likeonly for y-axis inversion cleanly separates datetime from timedelta axes, which should fix the timedelta y-axis formatting issue without changing the seismic-style inversion behavior. Wrappingimshowinmpl.rc_context({"image.resample": True})and threading theinterpolationparameter into theimshowcall is also a good, locally-scoped way to control resampling and interpolation behavior for waterfall plots only.Also applies to: 253-268
dascore/proc/align.py (1)
165-176: Input validation and high-level flow look sound.The expanded error message in
_validate_alignment_inputsthat includes the available coord names is helpful for debugging. The new flow inalign_to_coord(validate →_get_coord_valuesfor dtype compatibility →_get_shift_indices→_calculate_shift_info→_apply_shifts_to_data→_get_aligned_coords) is cohesive and keeps validation and transformation concerns nicely separated.Also applies to: 358-366, 373-374
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dascore/proc/align.py (1)
192-210: Fixnp.signon time-like coordinates in_get_shift_indices(still broken fortimedelta64)When
coord_valsis time-like (e.g.timedelta64),np.sign(coord_vals)returns a time-like array, andsign_vals.astype(np.int64)interprets those as underlying units (e.g. nanoseconds), not-1/0/1. This produces huge, incorrect integer shift values and breaks alignment for the timedelta-based example in the docstring and any time-like alignment coordinate. This is the same issue previously flagged and remains unresolved.A safer implementation is to compute the sign explicitly as integers:
-def _get_shift_indices(coord_vals, dim, reverse, samples): - """ - Get the indices for shifting. - - Positive values indicate a shift to the right, negative to left. - """ - # Relative means something slightly different here. It only means - # move backwards, rather than implying a reference from the end of the trace. - # Therefore, we need to insure positive values go into get_next_index. - abs_vals = np.abs(coord_vals) - sign_vals = np.sign(coord_vals) - inds_abs = dim.get_next_index( - abs_vals, - samples=samples, - relative=False if samples else True, - ) - # Reverse index if needed. This way the reference stays the same. - inds = inds_abs * (sign_vals.astype(np.int64) * (-1 if reverse else 1)) - return inds +def _get_shift_indices(coord_vals, dim, reverse, samples): + """ + Get the indices for shifting. + + Positive values indicate a shift to the right, negative to left. + """ + abs_vals = np.abs(coord_vals) + # Compute integer sign so it works for numeric and time-like dtypes. + sign_vals = np.zeros_like(abs_vals, dtype=np.int64) + sign_vals[coord_vals > 0] = 1 + sign_vals[coord_vals < 0] = -1 + inds_abs = dim.get_next_index( + abs_vals, + samples=samples, + relative=False if samples else True, + ) + factor = -1 if reverse else 1 + inds = inds_abs * (sign_vals * factor) + return indsIf you want, you can double-check NumPy’s current behavior for
np.signontimedelta64to confirm this reasoning.What dtype and values does `numpy.sign` return when applied to a `numpy.timedelta64` array, and how does casting that result to `int64` behave?
🧹 Nitpick comments (1)
dascore/proc/align.py (1)
213-241: New_get_coord_valueshelper solidly centralizes dtype validationThis helper cleanly enforces that
samples=Trueonly works with integer, non-time-like coordinates and that non-sample alignment values are cast/validated against the dimension’s step dtype usingget_compatible_valuesandnp.can_cast. That should prevent many subtle dtype mismatches between alignment coords and the aligned dimension.If you want to tighten it further, two small, optional tweaks:
- Call
dtype_time_like(dim.dtype)/dtype_time_like(coord.dtype)explicitly for readability.- Use
cdtypein thesamples=Trueerror message instead ofcoord.dtypeto guarantee consistency if those ever diverge.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/proc/align.py(6 hunks)dascore/viz/waterfall.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dascore/viz/waterfall.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T16:22:22.879Z
Learnt from: d-chambers
Repo: DASDAE/dascore PR: 523
File: dascore/transform/fourier.py:433-434
Timestamp: 2025-09-02T16:22:22.879Z
Learning: In dascore STFT implementation, use `require_evenly_sampled=True` on `get_coord` calls instead of manual validation for coord.step to ensure sampling rate calculations are safe.
Applied to files:
dascore/proc/align.py
🧬 Code graph analysis (1)
dascore/proc/align.py (3)
dascore/core/patch.py (5)
coords(255-269)dtype(318-320)shape(286-299)data(272-283)ndim(229-231)dascore/core/coords.py (14)
get_compatible_values(170-188)get_coord(1533-1724)min(438-440)update(742-755)update(983-985)change_length(893-908)change_length(1048-1053)change_length(1287-1298)get_next_index(795-873)values(1000-1004)values(1259-1274)index(702-721)data(606-608)ndim(468-470)dascore/utils/time.py (1)
dtype_time_like(419-429)
⏰ 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.13)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.12)
- 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 (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_build_docs
- GitHub Check: Run benchmarks
🔇 Additional comments (3)
dascore/proc/align.py (3)
160-166: Error message clarification looks goodUsing
tuple(patch.coords.coord_map)makes the coords listing in the error message much clearer and avoids dumping the raw mapping representation. No issues here.
179-189: Aligned coordinate computation is consistent with shift metadataUsing
coord.min() + coord.step * start_shifttogether withchange_length(len(coord) - start_shift + end_shift)keeps the updated dimension length in sync with_ShiftInfo.output_lengthand preserves even sampling. This assumes the aligned dimension is evenly sampled, which is enforced byrequire_evenly_sampled=True, so this looks correct.
353-375: Core wiring ofalign_to_coordlooks correct and aligns with prior best practicesUsing
_validate_alignment_inputs,patch.get_coord(dim_name, require_evenly_sampled=True),_get_coord_values,_get_shift_indices, and_get_aligned_coordsgives a clear, linear pipeline: validate → normalize values → compute shifts → compute metadata → shift data → update coords. The evenly-sampled requirement is appropriate here, since both_get_coord_valuesand_get_aligned_coordsrely on a well-defined step, and it matches the prior learning to preferrequire_evenly_sampled=Trueover ad-hoc step checks.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dascore/proc/align.py(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T16:22:22.879Z
Learnt from: d-chambers
Repo: DASDAE/dascore PR: 523
File: dascore/transform/fourier.py:433-434
Timestamp: 2025-09-02T16:22:22.879Z
Learning: In dascore STFT implementation, use `require_evenly_sampled=True` on `get_coord` calls instead of manual validation for coord.step to ensure sampling rate calculations are safe.
Applied to files:
dascore/proc/align.py
🧬 Code graph analysis (1)
dascore/proc/align.py (2)
dascore/core/coords.py (14)
get_compatible_values(170-188)get_coord(1533-1724)min(438-440)update(742-755)update(983-985)change_length(893-908)change_length(1048-1053)change_length(1287-1298)get_next_index(795-873)values(1000-1004)values(1259-1274)index(702-721)data(606-608)ndim(468-470)dascore/utils/time.py (1)
dtype_time_like(419-429)
⏰ 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). (19)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- 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 (windows-latest, 3.13)
- 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_build_docs
- GitHub Check: Run benchmarks
🔇 Additional comments (5)
dascore/proc/align.py (5)
14-14: LGTM: New imports support coordinate validation.The
get_compatible_valuesanddtype_time_likeimports are correctly added to support the new_get_coord_valuesvalidation function.Also applies to: 18-18
179-189: LGTM: Simplified coordinate alignment logic.The signature simplification and coordinate calculation correctly implement the new relative-only semantics. Computing
new_minascoord.min() + coord.step * start_shiftproperly adjusts the coordinate range based on the shift metadata.
254-357: LGTM: Docstring correctly updated for relative-only semantics.The docstring has been properly revised to reflect the new purely relative alignment behavior:
- The
samplesparameter now clearly documents the two supported modes without referencing the removedrelativeparameter- Examples use neutral naming (e.g.,
aligned_offsets) instead of absolute/relative terminology- The new "Alignment Semantics" section (lines 352-357) explicitly clarifies the relative-only behavior
364-382: LGTM: Function calls correctly updated for refactored flow.The integration of the refactored helper functions is correct:
- Requiring evenly sampled dimensions (line 364) is necessary for the step-based shift calculations
- The new validation step (line 367) ensures coordinate compatibility before computing shifts
- Updated function calls pass the correct validated values and simplified parameters
The TODO comment (lines 362-363) about relaxing the evenly sampled requirement for certain modes is a reasonable future enhancement.
213-241: Code validation logic is correct—no issues found.The dtype validation in
_get_coord_valuesproperly handles coordinate and dimension type checking. The function correctly:
- Rejects non-integer coordinates when
samples=True(line 219)- Computes
expected_dtypeas the result of dimension value differences (line 229), which converts datetime64 to timedelta64- Applies
get_compatible_valuesconversion only when the dimension is time-like but the coordinate is not (line 233)- Validates that the final coordinate dtype can safely cast to the expected dtype (line 234)
The test cases confirm expected behavior:
bad_type_shift(datetime64) raises an error because datetime64 cannot safely cast to the expected timedelta64, andshift_time_relative(timedelta64) correctly raises when used withsamples=True.
Description
This PR changes
align_to_coord(see #561 and #578) so that it has more validation checks and only works in a "relative" mode, which makes more sense as the mixing usage of relative and absolute didn't quite fit. This is a breaking change, but sincealign_to_coordis only in master it doesn't matter.This also modifies the new waterfall plot (#574) such that
timedelta64y axis are not formatted to look like datetime64 axis.Checklist
I have (if applicable):
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.