allow chunking to drop non-dimensional coords #532
Conversation
WalkthroughAdds an optional Changes
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 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 (3)
⏰ 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). (13)
✨ Finishing Touches
🧪 Generate unit tests
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #532 +/- ##
=======================================
Coverage 99.88% 99.88%
=======================================
Files 122 122
Lines 10035 10040 +5
=======================================
+ Hits 10023 10028 +5
Misses 12 12
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dascore/utils/coordmanager.py (1)
114-129: Fix incorrect coord selection in _get_merged_coords (uses dim instead of coord_name).
This produces wrong concatenation when merging non-dim coordinates aligned to the merge dim. It also sets dims from the dim key instead of the coordinate being merged.- for coord_name in coords_to_merge: - merge_coords = [x.coord_map[dim] for x in managers] + for coord_name in coords_to_merge: + merge_coords = [x.coord_map[coord_name] for x in managers] axis = managers[0].dim_map[coord_name].index(dim) @@ - dims = managers[0].dim_map[dim] + dims = managers[0].dim_map[coord_name]Consider adding a unit test that includes an extra non-dimensional coordinate (sharing the merge dim) to catch this path.
🧹 Nitpick comments (6)
dascore/utils/coordmanager.py (3)
21-22: Type hint the new parameter for clarity and API hygiene.
Add a bool annotation to drop_conflicting.- drop_conflicting=False, + drop_conflicting: bool = False,
24-24: Fix docstring typo.
"Merger" → "Merge".- Merger coordinate managers along a specified dimension. + Merge coordinate managers along a specified dimension.
80-85: Use the public API name in the error hint.
spool.chunk takes conflict=..., not conflicts=....- "Coordinate managers cannot be merged. Try using " - "spool.chunk with conflicts='drop'." + "Coordinate managers cannot be merged. Try using " + "spool.chunk with conflict='drop'."tests/test_core/test_patch_chunk.py (3)
273-287: Optional: prefer Generator over legacy RandomState.
Switching to numpy.random.default_rng improves reproducibility patterns going forward.- rand = np.random.RandomState(42) - c1 = rand.random(random_patch.shape[dist_ax]) - c2 = rand.random(c1.shape) + rng = np.random.default_rng(42) + c1 = rng.random(random_patch.shape[dist_ax]) + c2 = rng.random(c1.shape)
485-490: Fix test docstring (behavior without conflict='drop').
Should raise, not drop.- Otherwise they should be dropped. + Otherwise it should raise.
496-497: Tiny readability nit.
Use generator form of any().- assert not any([x.startswith("_") for x in coord_names]) + assert not any(x.startswith("_") for x in coord_names)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dascore/utils/coordmanager.py(3 hunks)dascore/utils/patch.py(2 hunks)tests/test_core/test_patch_chunk.py(2 hunks)tests/test_utils/test_coordmanager_utils.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
dascore/utils/coordmanager.py (1)
dascore/core/patch.py (1)
dims(188-190)
dascore/utils/patch.py (1)
dascore/utils/coordmanager.py (1)
merge_coord_managers(17-146)
tests/test_core/test_patch_chunk.py (6)
dascore/examples.py (1)
random_patch(29-111)tests/conftest.py (2)
random_patch(282-284)spool(569-571)dascore/core/coords.py (2)
get_coord(1475-1663)coord_range(509-529)dascore/proc/coords.py (2)
get_coord(87-139)update_coords(218-243)dascore/core/spool.py (3)
spool(670-699)chunk(111-158)chunk(511-541)dascore/exceptions.py (1)
CoordMergeError(42-43)
tests/test_utils/test_coordmanager_utils.py (5)
tests/conftest.py (1)
cm_basic(106-108)dascore/core/coords.py (5)
index(698-717)get_coord(1475-1663)update(738-751)update(925-927)coord_range(509-529)dascore/core/coordmanager.py (3)
get_coord(990-1009)update(230-286)coord_range(1031-1033)dascore/utils/coordmanager.py (1)
merge_coord_managers(17-146)dascore/exceptions.py (1)
CoordMergeError(42-43)
⏰ 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). (17)
- GitHub Check: test_code (windows-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.11)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-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 (ubuntu-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)
🔇 Additional comments (3)
dascore/utils/patch.py (2)
395-400: LGTM: plumbs drop_conflicting through to merge_coord_managers.
Wiring is correct; snap behavior remains unchanged.
422-425: Aligndrop_conf_coordsto only drop onconflicts='drop'
Current logic also treatskeep_firstas a drop, which extends behavior beyond the PR’s intent. Update to:- drop_conf_coords = True if conf in {"drop", "keep_first"} else False + drop_conf_coords = conf == "drop"If
keep_firstis meant to drop non-dimensional coord conflicts as well, update the documentation to state this explicitly.tests/test_utils/test_coordmanager_utils.py (1)
163-167: LGTM: exercises both drop and non-drop paths.
Covers the new flag and the error branch.
Description
This PR allows
patch.chunkto drop conflicting, non dimensional coordinates whenconflict = 'drop'Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Documentation
Tests