fix #583 #584
fix #583 #584
Conversation
WalkthroughDataFrameSpool iteration now catches IndexError: when the message indicates an "out of bounds" coordinate mismatch it emits a UserWarning and skips that patch; other IndexError cases are re-raised. Iterator docstring expanded and tests added for differing-distance spools and IndexError behavior. Changes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/core/spool.py(2 hunks)tests/test_clients/test_dirspool.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_clients/test_dirspool.py (7)
dascore/examples.py (2)
random_patch(29-111)spool_to_directory(635-655)tests/conftest.py (2)
random_patch(282-284)spool(569-571)dascore/proc/coords.py (2)
get_coord(96-148)select(433-525)dascore/core/coords.py (9)
get_coord(1533-1724)select(323-329)select(1015-1035)select(1167-1192)select(1338-1364)select(1449-1474)max(443-445)update(742-755)update(983-985)dascore/core/coordmanager.py (4)
get_coord(1039-1058)select(594-636)max(1064-1066)update(240-308)dascore/core/spool.py (4)
select(145-167)select(557-576)spool(666-695)update(238-250)dascore/clients/dirspool.py (1)
update(105-112)
⏰ 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 (windows-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-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 (macos-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: Run benchmarks
🔇 Additional comments (2)
dascore/core/spool.py (1)
7-7: LGTM: Standard library import.The
suppresscontext manager is appropriate for selectively suppressing exceptions during iteration.tests/test_clients/test_dirspool.py (1)
500-504: Good practice: xfail test documents expected future behavior.This xfail test appropriately documents the intended behavior once proper indexing is implemented, making it clear that the current fix is temporary.
tests/test_clients/test_dirspool.py
Outdated
| def test_differing_distances(self, dist_differ_spool, random_patch): | ||
| """Ensure iteration still works with conditions described in #538.""" | ||
| dist = random_patch.get_coord("distance") | ||
| out = dist_differ_spool.select(distance=(dist.max() / 2, ...)) | ||
| # #583 would raise on iterating. If this doesn't raise the test pases. | ||
| # We don't want to test the len of the spool, because it should actually | ||
| # be 1, but that won't be the case until we redo the indexing. | ||
| for _ in out: | ||
| pass |
There was a problem hiding this comment.
Fix typos and strengthen test assertions.
The test has the following issues:
- Issue reference typo: Docstring references "warning for chunking gappy spools #538" but should reference "Spool iteration not robust to files changing distance #583"
- Spelling typo: Comment says "pases" should be "passes"
- Weak assertion: The test only verifies that iteration doesn't raise an exception, but doesn't assert anything about the results
Apply this diff to fix the typos and add a basic assertion:
def test_differing_distances(self, dist_differ_spool, random_patch):
- """Ensure iteration still works with conditions described in #538."""
+ """Ensure iteration still works with conditions described in #583."""
dist = random_patch.get_coord("distance")
out = dist_differ_spool.select(distance=(dist.max() / 2, ...))
- # #583 would raise on iterating. If this doesn't raise the test pases.
+ # #583 would raise on iterating. If this doesn't raise the test passes.
# We don't want to test the len of the spool, because it should actually
# be 1, but that won't be the case until we redo the indexing.
- for _ in out:
- pass
+ # Verify iteration completes without raising
+ patches = list(out)
+ # At least one patch should be yielded if selection is valid
+ assert len(patches) >= 0, "Iteration should complete without raising"📝 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_differing_distances(self, dist_differ_spool, random_patch): | |
| """Ensure iteration still works with conditions described in #538.""" | |
| dist = random_patch.get_coord("distance") | |
| out = dist_differ_spool.select(distance=(dist.max() / 2, ...)) | |
| # #583 would raise on iterating. If this doesn't raise the test pases. | |
| # We don't want to test the len of the spool, because it should actually | |
| # be 1, but that won't be the case until we redo the indexing. | |
| for _ in out: | |
| pass | |
| def test_differing_distances(self, dist_differ_spool, random_patch): | |
| """Ensure iteration still works with conditions described in #583.""" | |
| dist = random_patch.get_coord("distance") | |
| out = dist_differ_spool.select(distance=(dist.max() / 2, ...)) | |
| # #583 would raise on iterating. If this doesn't raise the test passes. | |
| # We don't want to test the len of the spool, because it should actually | |
| # be 1, but that won't be the case until we redo the indexing. | |
| # Verify iteration completes without raising | |
| patches = list(out) | |
| # At least one patch should be yielded if selection is valid | |
| assert len(patches) >= 0, "Iteration should complete without raising" |
🤖 Prompt for AI Agents
In tests/test_clients/test_dirspool.py around lines 490 to 498, fix the
docstring and comment typos and strengthen the test: change the docstring issue
reference from "#538" to "#583", correct "pases" to "passes", and instead of
only iterating, collect the results into a list (e.g., items = list(out)) and
add a basic assertion such as assert len(items) > 0 (optionally also assert that
each item's distance meets the selection condition) so the test verifies output
as well as that iteration does not raise.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #584 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 129 129
Lines 11207 11233 +26
=======================================
+ Hits 11199 11225 +26
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:
|
CodSpeed Performance ReportMerging #584 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_clients/test_dirspool.py (1)
491-499: Consider removing redundant select and strengthening assertions.The test has two minor issues:
- Redundant select: Line 494 re-applies the same distance selection that's already applied in the
dist_differ_spoolfixture (line 431), which is unnecessary.- Weak assertions: The test only verifies the warning is issued but doesn't assert anything about the iteration results (e.g., that iteration completes, what patches were yielded, or that yielded patches satisfy the selection).
Consider this refactor:
def test_differing_distances(self, dist_differ_spool, random_patch): """Ensure iteration still works with conditions described in #583.""" - dist = random_patch.get_coord("distance") - out = dist_differ_spool.select(distance=(dist.max() / 2, ...)) # #583 would raise on iterating. Verify that a warning is issued when # a patch is skipped due to coordinate mismatch. with pytest.warns(UserWarning, match="Skipping patch at index.*#583"): - for _ in out: - pass + patches = list(dist_differ_spool) + # Verify iteration completed and yielded patches + assert len(patches) > 0, "Should yield at least one valid patch" + # Verify the yielded patches satisfy the distance selection + dist = random_patch.get_coord("distance") + for patch in patches: + patch_dist = patch.get_coord("distance") + assert patch_dist.min() >= dist.max() / 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/core/spool.py(3 hunks)tests/test_clients/test_dirspool.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dascore/core/spool.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_clients/test_dirspool.py (3)
tests/conftest.py (4)
patch(375-377)random_patch(282-284)spool(569-571)basic_file_spool(500-503)dascore/utils/misc.py (1)
suppress_warnings(82-94)dascore/examples.py (2)
random_patch(29-111)spool_to_directory(635-655)
🔇 Additional comments (4)
tests/test_clients/test_dirspool.py (4)
6-6: LGTM!The new imports are properly used in the test methods below.
Also applies to: 19-19
419-431: LGTM!The fixture correctly sets up the test scenario for issue #583 by creating a spool with two patches having incompatible distance ranges, then applying a distance selection that only one patch can satisfy.
501-506: LGTM!The xfail marker is appropriate for documenting the intended future behavior once the new indexing implementation is complete. The test clearly communicates expectations.
508-520: LGTM!The test correctly verifies that only IndexErrors with "out of bounds" messages (specific to issue #583) are caught and converted to warnings, while other IndexErrors are properly re-raised. The TODO comment appropriately marks this as temporary code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_clients/test_dirspool.py (1)
419-431: Consider enhancing fixture documentation.The fixture correctly sets up the #583 scenario, but the docstring and code could be clearer about the specific incompatibility being created: patch2 has a distance range [0, max/4], while the selection requests distance ≥ max/2, making patch2's data unreachable.
Consider this enhancement:
@pytest.fixture(scope="class") def dist_differ_spool(self, tmp_path_factory, random_patch): - """Setup conditions for testing #583""" + """ + Setup conditions for testing #583: spool with incompatible distance ranges. + + Creates two patches where one (patch2) has a distance range [0, max/4] + that is incompatible with the selection distance >= max/2. + """ out_path = tmp_path_factory.mktemp("multi_dis_dir") distance = random_patch.get_coord("distance") time = random_patch.get_coord("time") + # Create patch2 with limited distance range that won't satisfy selection patch2 = random_patch.select(distance=(None, distance.max() / 4)).update_coords( time_min=time.max() ) spool = dc.spool([random_patch, patch2]) dascore.examples.spool_to_directory(spool, path=out_path) + # Select distance range that excludes patch2 dist_range = (distance.max() / 2, ...) return dc.spool(out_path).update().select(distance=dist_range)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_clients/test_dirspool.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_clients/test_dirspool.py (3)
tests/conftest.py (4)
patch(375-377)random_patch(282-284)spool(569-571)basic_file_spool(500-503)dascore/utils/misc.py (1)
suppress_warnings(82-94)dascore/examples.py (2)
random_patch(29-111)spool_to_directory(635-655)
⏰ 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.12)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- 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 (ubuntu-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- 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: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-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 (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: Run benchmarks
🔇 Additional comments (4)
tests/test_clients/test_dirspool.py (4)
6-6: LGTM: Imports are appropriate.Both new imports are used in the tests below:
upatchfor mocking intest_iteration_unexpected_index_errorandsuppress_warningsfor the xfail test.Also applies to: 19-19
491-499: LGTM: Test correctly verifies the #583 fix.The test appropriately verifies that:
- Iteration completes without raising IndexError
- A UserWarning is issued when incompatible patches are skipped
- Yielded patches are valid Patch instances
The assertion at line 493 checks only that the spool is non-empty rather than verifying exact counts, which is intentional as noted in the related xfail test—the current indexing allows incompatible patches in the spool length.
500-506: LGTM: Appropriate use of xfail to document intended behavior.This xfail test correctly documents the expected behavior once robust indexing is implemented: incompatible patches should be excluded from the spool length rather than skipped during iteration. The use of
suppress_warningsis appropriate to avoid noise from the temporary #583 fix.
507-522: LGTM: Robust test for error handling.This test appropriately verifies that only #583-specific IndexErrors are suppressed during iteration, while other unexpected IndexErrors are re-raised. The mocking approach is sound, and the TODO comment correctly notes this can be removed once robust indexing is implemented.
Description
Fixes #583 by skipping problematic patches in spool iteration. This is a temporary fix until a more robust indexing is implemented.
Checklist
I have (if applicable):
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.