raise error when select on bad patch coord #556
Conversation
WalkthroughAdded runtime validation to raise Changes
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
🔇 Additional comments (1)
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 (2)
dascore/proc/coords.py (1)
505-513: Good pre-validation for invalid coordinate kwargs; consider mirroring in order().This prevents silent no-ops and surfaces user typos early. For consistency, apply the same invalid-kwargs check in order() so both APIs behave identically when given unknown coordinates.
Apply this diff to order():
def order( patch: PatchType, *, copy=False, relative=False, samples=False, **kwargs ) -> PatchType: """ Re-order the patch contents based on coordinate values or indices. @@ - new_coords, data = patch.coords.order( + # Check for and raise on invalid kwargs. + if invalid_coords := set(kwargs) - set(patch.coords.coord_map): + invalid_list = sorted(invalid_coords) + valid_list = sorted(patch.coords.coord_map) + msg = ( + f"Coordinate(s) {invalid_list} not found in patch coordinates: {valid_list}" + ) + raise PatchCoordinateError(msg) + + new_coords, data = patch.coords.order( **kwargs, array=patch.data, relative=relative, samples=samples, )Optional polish in the select docstring while you’re here:
- Fix typos “possitive” → “positive”, “distace” → “distance”.
- If True, select ranges are relative to the start of coordinate, if - possitive, or the end of the coordinate, if negative. + If True, select ranges are relative to the start of coordinate, if + positive, or the end of the coordinate, if negative. @@ - >>> new_distance_3 = patch.select(distace=distance[1::2]) + >>> new_distance_3 = patch.select(distance=distance[1::2])tests/test_proc/test_proc_coords.py (1)
335-342: Make regex raw and precise to satisfy Ruff RUF043 and reduce false positives.Use a raw string and match the full invalid keys used in the call.
Based on hints
- with pytest.raises(PatchCoordinateError, match="coord1.*coord2"): - random_patch.select(bad_coord1=(0, 10), bad_coord2=(5, 15)) + with pytest.raises(PatchCoordinateError, match=r"bad_coord1.*bad_coord2"): + random_patch.select(bad_coord1=(0, 10), bad_coord2=(5, 15))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/proc/coords.py(2 hunks)tests/test_proc/test_proc_coords.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dascore/proc/coords.py (2)
dascore/exceptions.py (4)
CoordError(38-39)ParameterError(26-27)PatchCoordinateError(58-59)PatchError(30-31)dascore/core/patch.py (1)
coords(255-269)
tests/test_proc/test_proc_coords.py (2)
dascore/exceptions.py (1)
PatchCoordinateError(58-59)dascore/proc/coords.py (1)
select(433-525)
🪛 Ruff (0.13.1)
tests/test_proc/test_proc_coords.py
340-340: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
⏰ 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: Run benchmarks
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- 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_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.12)
🔇 Additional comments (4)
dascore/proc/coords.py (1)
13-18: Importing PatchCoordinateError is correct.Matches the new pre-validation behavior in select and is used below.
tests/test_proc/test_proc_coords.py (3)
17-18: LGTM: importing PatchCoordinateError for new assertions.
325-333: Test covers single invalid coordinate well.Asserts error type and message fragment; good.
343-351: LGTM: mixed valid/invalid coordinates should error before slicing.
CodSpeed Performance ReportMerging #556 will not alter performanceComparing Summary
|
coderabbitai
bot
added
patch
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #556 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 126 126
Lines 10472 10481 +9
=======================================
+ Hits 10464 10473 +9
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/hampel.py (1)
44-46: Changing separable default to True is a behavioral change; align docs and confirm intent
- This may change results/perf for multi-dim windows; confirm this is intended for a minor release.
- The Warning section still suggests “consider using separable=True”, which is now the default. Update wording to reflect the new default.
Proposed doc tweak:
- Warning - ------- - Selecting windows with many samples can be *very* slow. It is recommended - window size in each dimension be <10 samples. For larger windows, consider - using `separable=True` for significantly faster processing (3-4x speedup) - at the cost of a slight approximation. + Warning + ------- + Selecting windows with many samples can be slow. The default `separable=True` + applies 1D median filters sequentially (~3–4x speedup) with a slight approximation. + Set `separable=False` to compute the true N‑D median when exactness is required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/proc/coords.py(3 hunks)dascore/proc/hampel.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dascore/proc/coords.py (2)
dascore/exceptions.py (4)
CoordError(38-39)ParameterError(26-27)PatchCoordinateError(58-59)PatchError(30-31)dascore/core/patch.py (1)
coords(255-269)
⏰ 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.10)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: Run benchmarks
🔇 Additional comments (3)
dascore/proc/coords.py (2)
13-18: Importing PatchCoordinateError looks correctBrings the right exception into scope for select validation.
495-496: Doc example fix LGTMThe distance selection example reads correctly.
dascore/proc/hampel.py (1)
100-101: Example update LGTMShorter single-dimension window demonstrates usage clearly with the new defaults.
| # Check for and raise on invalid kwargs. | ||
| if invalid_coords := set(kwargs) - set(patch.coords.coord_map): | ||
| invalid_list = sorted(invalid_coords) | ||
| valid_list = sorted(patch.coords.coord_map) | ||
| msg = ( | ||
| f"Coordinate(s) {invalid_list} not found in patch coordinates: {valid_list}" | ||
| ) | ||
| raise PatchCoordinateError(msg) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate against dimensions (not just coordinates) to meet PR objective
Current check only guards unknown coordinates; it still allows non-dimensional coordinates, which conflicts with “raise when a keyword that is not a dimension is passed.” Recommend validating both unknown and non-dim keys.
Apply this diff:
- # Check for and raise on invalid kwargs.
- if invalid_coords := set(kwargs) - set(patch.coords.coord_map):
- invalid_list = sorted(invalid_coords)
- valid_list = sorted(patch.coords.coord_map)
- msg = (
- f"Coordinate(s) {invalid_list} not found in patch coordinates: {valid_list}"
- )
- raise PatchCoordinateError(msg)
+ # Validate selection keys: first unknown, then non-dimensional.
+ keys = set(kwargs)
+ unknown = keys - set(patch.coords.coord_map)
+ if unknown:
+ invalid_list = sorted(unknown)
+ valid_list = sorted(patch.coords.coord_map)
+ msg = f"Coordinate(s) {invalid_list} not found in patch coordinates: {valid_list}"
+ raise PatchCoordinateError(msg)
+ non_dim = keys - set(patch.dims)
+ if non_dim:
+ invalid_list = sorted(non_dim)
+ valid_list = list(patch.dims)
+ msg = f"Only dimensions can be used in select. Got {invalid_list}; valid dimensions are {valid_list}"
+ raise PatchCoordinateError(msg)Optional: mirror the same validation in order(...) for consistency.
📝 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.
| # Check for and raise on invalid kwargs. | |
| if invalid_coords := set(kwargs) - set(patch.coords.coord_map): | |
| invalid_list = sorted(invalid_coords) | |
| valid_list = sorted(patch.coords.coord_map) | |
| msg = ( | |
| f"Coordinate(s) {invalid_list} not found in patch coordinates: {valid_list}" | |
| ) | |
| raise PatchCoordinateError(msg) | |
| # Validate selection keys: first unknown, then non-dimensional. | |
| keys = set(kwargs) | |
| unknown = keys - set(patch.coords.coord_map) | |
| if unknown: | |
| invalid_list = sorted(unknown) | |
| valid_list = sorted(patch.coords.coord_map) | |
| msg = f"Coordinate(s) {invalid_list} not found in patch coordinates: {valid_list}" | |
| raise PatchCoordinateError(msg) | |
| non_dim = keys - set(patch.dims) | |
| if non_dim: | |
| invalid_list = sorted(non_dim) | |
| valid_list = list(patch.dims) | |
| msg = f"Only dimensions can be used in select. Got {invalid_list}; valid dimensions are {valid_list}" | |
| raise PatchCoordinateError(msg) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dascore/core/coordmanager.py (1)
120-143: Improve coordinate selection validation and handling for non-dimensional coordinates.The addition of runtime validation and support for non-dimensional coordinates represents a significant improvement. The assertion on line 122 ensures coordinate existence, and the handling of coordinates with empty
coord_dims(lines 127-133) is well-implemented. However, the redundant call to_ensure_1d_coordon line 135 should be removed.Apply this diff to remove the redundant coordinate validation:
# Handle dimensional coordinates (tied to exactly one dimension) - _ensure_1d_coord(coord, coord_name) dim_name = coord_dims[0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dascore/core/coordmanager.py(1 hunks)dascore/proc/coords.py(3 hunks)dascore/proc/hampel.py(1 hunks)tests/test_core/test_coordmanager.py(1 hunks)tests/test_proc/test_proc_coords.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_core/test_coordmanager.py
🚧 Files skipped from review as they are similar to previous changes (1)
- dascore/proc/coords.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_proc/test_proc_coords.py (3)
dascore/exceptions.py (1)
PatchCoordinateError(58-59)dascore/core/coordmanager.py (5)
update(240-308)new(453-471)get_array(1072-1074)shape(793-799)select(594-636)dascore/proc/coords.py (2)
get_array(151-200)select(433-525)
🪛 Ruff (0.13.1)
tests/test_proc/test_proc_coords.py
357-357: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
⏰ 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 (ubuntu-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: Run benchmarks
- 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 (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
🔇 Additional comments (4)
tests/test_proc/test_proc_coords.py (4)
17-17: LGTM! Import of PatchCoordinateError added.The import of
PatchCoordinateErrorfromdascore.exceptionsis correctly added to support the new error handling functionality.
342-368: Add validation for non-existent coordinate selection.The test cases correctly validate that
PatchCoordinateErroris raised when selecting non-existent coordinates. This ensures the new runtime validation works as expected for single, multiple, and mixed coordinate scenarios.
369-550: Comprehensive test coverage for non-dimensional coordinate selection.The test suite provides excellent coverage for non-dimensional coordinate selection scenarios including boolean masks, array indices, mixed dimensional/non-dimensional coordinates, and coordinates tied to dimensions. The helper methods (
_add_non_dim_coords,_assert_coord_unchanged,_assert_data_shape_unchanged) improve test readability and reduce duplication.
196-212: Helper methods enhance test organization.The helper methods improve test structure by encapsulating common operations for adding non-dimensional coordinates and asserting unchanged state. This promotes DRY principles and makes tests more maintainable.
Description
This PR partially addresses #435 by making the
Patch.selectraise an error when a keyword that is not a dimension is passed in. Fixing the spool will require much more work because the indexer may not know about all the dimensions in its managed patches.Checklist
I have (if applicable):
Summary by CodeRabbit
Bug Fixes
Tests
Documentation