Cover the CoordRange fast paths missed on dev - #790
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5999495d2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except ZeroDivisionError: | ||
| return None |
There was a problem hiding this comment.
When a length-one range has a Python-scalar zero step, returning None here treats every finite bound as unconstrained, so CoordRange(start=0, stop=0, step=0).select((1, 10)) silently returns the value at 0 rather than an empty coordinate; half-bounded selections such as (1, None) fail similarly. Handle the zero-step case by comparing the bound with the singleton value, and add disjoint-boundary coverage rather than testing only a range containing start. .agents/agents.mdL66-L72
Useful? React with 👍 / 👎.
The perf work in #778 left two lines uncovered in dascore/core/coords.py: - the ndarray branch of _round_ratio in the CoordRange validator, which is dead code (multi-element arrays are rejected earlier by the pd.isnull check, so the ratio is always scalar-like); it is removed. - the guard returning None when the index fraction isn't finite, i.e. a CoordRange with a step of 0. Selecting on such a coord with python (not numpy) scalars raised ZeroDivisionError rather than returning everything, which the array-based implementation prior to #778 did; that is fixed and both cases are now tested.
Every sample of such a coord equals start, so bounds which don't contain start now yield a degenerate selection instead of keeping the sample.
They spawn threads via run_in_threads, which fails in WebAssembly where the wasm suite deselects concurrency tests.
a500983 to
6cf424b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #790 +/- ##
===========================================
+ Coverage 99.98% 100.00% +0.01%
===========================================
Files 164 164
Lines 17707 17713 +6
===========================================
+ Hits 17705 17713 +8
+ Misses 2 0 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Description
Restores 100% coverage on
dev. The perf work in #778 left two uncovered lines indascore/core/coords.py, which every branch cut since then inherits (#784 and #786 both report the same two misses)._round_rationdarray branch (validator) — dead code. Multi-element arrays never reach it:validate_start_stop_step_lencallspd.isnullon start/stop/step/shape first, which raises on any array with more than one element. Only scalars, 0d arrays, and 1-element arrays (unboxed by_maybe_unbox_scalar) get that far, andfloat()handles all of them. Removed, with a test for the 0d-array input that used to take that branch.Non-finite fraction guard in
CoordRange._get_index— only reachable for aCoordRangewith a step of 0 (test_coord_range_len_1covers building one), and nothing selected on one. Doing so turned up two problems:CoordRange(start=0, stop=0, step=0).select((0, 10))raisedZeroDivisionError. Numpy scalars instead giveinf/nanand hit the guard. Both are now handled.start(.valuesis[start]), but ondev/masterselect((0, 0))returned an empty coord whileselect((1, None))kept the sample — an artifact ofinf/nancasting toINT64_MIN, not a contract._get_zero_step_indexnow gives these coords real single-value semantics: bounds containingstartkeep the sample, bounds that don't select nothing.The zero step is detected after the division rather than up front because testing a numpy step for truthiness costs ~130 ns for
timedelta64vs ~12 ns formath.isfinite, and this is a hot path for time-coordinate selection.Also marks the three thread-spawning tests added in #781 (
TestRemoteCacheConcurrencyandTestIOResourceManagerConcurrencyintests/test_utils/test_io_utils.py) withconcurrency, which they were missing. They fail withRuntimeError: can't start new threadin the WebAssembly suite added by #783, which deselectsnot network and not concurrency. An AST sweep overtests/confirms these were the only thread-using tests without the mark.Changelog
none
Checklist
I have (if applicable):