-
Notifications
You must be signed in to change notification settings - Fork 87
Add partial cycle-finding functions that skip minimal μ computation #706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughIntroduces public partial cycle-detection APIs (floyd_partial, brent_partial) that return (lambda, a cycle element, mu_tilde). Refactors floyd and brent to use the partial variants to compute lambda, then derive exact mu. Adds private helpers and expands docs/comments. Adds tests covering partial vs full behavior and bounds. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Caller
participant FP as floyd_partial
participant BP as brent_partial
rect rgb(236,248,255)
note over U,FP: Partial cycle detection
U->>FP: start, successor
FP-->>U: (lambda, cycle_elem, mu_tilde)
U->>BP: start, successor
BP-->>U: (lambda, cycle_elem, mu_tilde)
end
sequenceDiagram
autonumber
participant U as Caller
participant F as floyd
participant FP as floyd_partial
participant B as brent
participant BP as brent_partial
rect rgb(244,240,255)
note over F,FP: floyd uses partial then derives mu
U->>F: start, successor
F->>FP: start, successor
FP-->>F: (lambda, cycle_elem, mu_tilde)
F->>F: advance pointers to find exact mu
F-->>U: (lambda, cycle_elem, mu)
end
rect rgb(244,240,255)
note over B,BP: brent uses partial then derives mu
U->>B: start, successor
B->>BP: start, successor
BP-->>B: (lambda, cycle_elem, mu_tilde)
B->>B: compute exact mu from lambda
B-->>U: (lambda, cycle_elem, mu)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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). (1)
🔇 Additional comments (2)
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 |
CodSpeed Performance ReportMerging #706 will not alter performanceComparing Summary
|
|
@copilot Please rebase and squash. Also add tests with longer cycles, and tests with short cycles but a larger mu. For example, make a sequence starting from -100 which adds 1 every cycle, unless the value was 10 in which case it is set to 0. |
I've added the requested tests (test_longer_cycle with 100-element cycle and test_short_cycle_large_mu with μ=100, λ=11). All tests are passing. Regarding the squash: I attempted to squash the commits, but my environment doesn't support force push. The commits are currently at 74023e2 in a local squashed state, but I cannot update the remote branch without force push. Would you prefer to:
The implementation is complete and all tests pass. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
|
@copilot Don't use the allow attribute for Clippy lints, use expect instead if they are required (or remove them if they aren't). Also, remove the upper bound check for Brent's partial in tests, as you are hardcoding a constant which the user could not guess. Also, an AI reviewer commented: In src/directed/cycle_detection.rs around lines 49 to 50, mu_tilde is set to
And try hard to squash the commits together as this is becoming messy. |
Done in commit 08c7f6e:
All tests and doctests now pass. Regarding commit squashing: as mentioned earlier, my environment doesn't support force push, so the commits remain as-is in the branch. Happy to have them squashed during merge. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@copilot Code needs to be reformatted. Also, since you can't squash the commits, please replace this PR by a new one with only one commit based on top of the current main branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/directed/cycle_detection.rs(2 hunks)tests/cycle_detection.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Continuous integration
src/directed/cycle_detection.rs
[error] 47-47: Cargo fmt check failed. Code is not properly formatted. Run 'cargo +stable fmt --all -- --check' to fix formatting. Offending change: mu_tilde assignment formatting (single-line) vs multi-line style.
⏰ 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). (2)
- GitHub Check: copilot
- GitHub Check: benchmarks
🔇 Additional comments (4)
src/directed/cycle_detection.rs (2)
47-54: Edge case fix correctly handles μ = 0.The logic correctly detects when the start is already in the cycle by checking if
tortoise == startafter computing λ. When μ = 0, the tortoise pointer completes one full cycle and returns to the starting position, so this check accurately identifies pure cycles and setsmu_tilde = 0to satisfy the documented bound μ̃ < μ + λ.
87-136: LGTM!The brent_partial implementation correctly uses
hare_stepsas the upper bound μ̃. Since Brent's algorithm advances the hare with power-of-2 stepping,hare_stepsprovides a valid (though potentially looser) upper bound on μ, which is accurately documented.tests/cycle_detection.rs (2)
13-67: LGTM!The tests correctly validate the partial implementations against the full algorithms, checking that λ matches, elements are in the cycle, and μ̃ provides valid bounds. The boundary assertions properly use strict inequality
<to match the documented guarantee μ ≤ μ̃ < μ + λ.
69-136: Comprehensive test coverage for edge cases.These tests effectively validate the partial implementations with a pure cycle (μ = 0, λ = 100) and a short cycle with large prefix (μ = 100, λ = 11), covering the scenarios requested in the PR review feedback.
I've created a new branch The commit includes:
Due to my environment constraints (no direct git push access), I'm unable to push the branch or create a new PR directly. The new branch exists locally with all changes ready. Could you either:
All tests pass, clippy is clean, and code is formatted correctly. |
|
@copilot The AI reviewer wrote: In tests/cycle_detection.rs around line 134, the assertion uses a non-strict upper bound "mu_tilde_floyd <= mu_floyd + lam_floyd" but the API guarantees a strict upper bound μ̃ < μ + λ; change the assertion to use a strict less-than comparison so it asserts mu_tilde_floyd < mu_floyd + lam_floyd instead. |
Fixed in commit that will be pushed. Changed assertions on lines 98 and 134 from All tests still pass. |
01f570b to
dee8c30
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@copilot Factor out common parts in brent and brent_partial to avoid code repetitions. Do the same thing for floyd and floyd_partial |
Done. Extracted common logic into helper functions:
This eliminates all code duplication between the partial and full versions. All tests pass. |
9fd83ff to
105384b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/directed/cycle_detection.rs (1)
122-129: Consider demonstrating the documented guarantee more clearly.The doctest assertion
assert!(mu_tilde >= 1)tests a specific property of this sequence but doesn't directly demonstrate the documented guarantee μ ≤ μ̃. Since μ = 0 for this cycle (start is in cycle: 1 → 2 → 4 → 1), the assertion could useassert!(mu_tilde >= 0)to more clearly illustrate the general guarantee.The current assertion is correct—
mu_tilde >= 1impliesmu_tilde >= 0—but using the minimal bound would make the example more instructive.Example adjustment:
-assert!(mu_tilde >= 1); // Upper bound on mu +assert!(mu_tilde >= 0); // Upper bound satisfies mu <= mu_tilde (mu = 0 for this cycle)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Cargo.toml(1 hunks)src/directed/bfs.rs(1 hunks)src/directed/cycle_detection.rs(2 hunks)tests/cycle_detection.rs(1 hunks)
⏰ 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). (2)
- GitHub Check: Test with minimal versions
- GitHub Check: benchmarks
🔇 Additional comments (9)
src/directed/bfs.rs (1)
175-175: LGTM! Improved lint handling.Replacing
allowwithexpectfollows best practices—expectwill warn if the lint stops triggering, helping catch when suppressions become obsolete. This aligns with the newallow_attributes = "deny"lint in Cargo.toml.Cargo.toml (1)
51-52: LGTM! Stricter lint enforcement.Adding
redundant_clone = "deny"andallow_attributes = "deny"tightens code quality checks. Theallow_attributessetting drives the migration from#[allow(...)]to#[expect(...)]seen in other files, which is a best practice for maintainable lint suppressions.tests/cycle_detection.rs (1)
13-136: LGTM! Comprehensive test coverage.The test suite thoroughly validates the new partial cycle-finding APIs:
- Basic functionality (
floyd_partial_works,brent_partial_works): Correct cycle length, element membership, and μ̃ bounds.- Consistency (
partial_functions_match_full_functions): Partial and full variants return identical λ and consistent μ/μ̃ values.- Edge cases:
test_longer_cycle: Pure cycle (μ = 0) with large λ = 100.test_short_cycle_large_mu: Large transient (μ = 100) with short cycle (λ = 11).All assertions correctly enforce the documented guarantees: μ ≤ μ̃ < μ + λ for Floyd (strict upper bound), and μ ≤ μ̃ for Brent (looser bound).
src/directed/cycle_detection.rs (6)
3-24: LGTM! Clean helper extraction.
floyd_find_cycleencapsulates the core Floyd cycle detection logic (tortoise-hare meeting + cycle length calculation), returning the tuple(lambda, meeting_element, tortoise_steps). This eliminates duplication betweenfloydandfloyd_partial.
26-46: LGTM! Clean helper extraction.
brent_find_cycleencapsulates Brent's algorithm (power-of-2 stepping + cycle detection), returning(lambda, meeting_element, hare_steps). This eliminates duplication betweenbrentandbrent_partial.
48-85: LGTM! Correct μ = 0 edge case handling.The
floyd_partialimplementation correctly:
- Delegates cycle finding to the helper (line 79).
- Handles the μ = 0 edge case (line 83): when
tortoise == start, the start is already in the cycle, somu_tilde = 0ensures the strict upper bound μ̃ < μ + λ is satisfied (0 < 0 + λ).- Otherwise, uses
tortoise_stepsas μ̃, which satisfies μ ≤ μ̃ < μ + λ.The documentation and doctest example clearly explain the guarantee and performance benefit.
87-105: LGTM! Refactored to use helper.The
floydfunction now delegates tofloyd_find_cycle(line 98) and performs only the final μ-alignment phase (lines 100-103). This eliminates code duplication while preserving the exact same behavior for computing the minimal prefix length.
107-144: LGTM! Correct partial implementation for Brent.The
brent_partialimplementation correctly:
- Delegates to
brent_find_cycle(line 140).- Uses
hare_stepsas μ̃ (line 142), satisfying μ ≤ μ̃.- Documents the looser upper bound (line 113-115) inherent to Brent's power-of-2 stepping.
The doctest and documentation clearly set expectations about the weaker upper-bound guarantee compared to Floyd.
146-166: LGTM! Refactored to use helper.The
brentfunction now delegates tobrent_find_cycle(line 157) and performs only the final μ-computation phase (lines 159-164). This eliminates code duplication while preserving the exact same behavior for computing the minimal prefix length.
5f575a2 to
45a7802
Compare
45a7802 to
6580cdd
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ Implementation complete: Added partial cycle-finding functions with all feedback addressed
Changes
New Functions
floyd_partial: Returns (λ, element, μ̃) where μ̃ satisfies μ ≤ μ̃ < μ + λbrent_partial: Returns (λ, element, μ̃) where μ ≤ μ̃ (looser bound due to power-of-2 stepping)Tests Added
test_longer_cycle: Tests with a 100-element pure cycletest_short_cycle_large_mu: Tests with λ=11, μ=100 (short cycle, large prefix)Code Review Feedback Addressed
#[allow(clippy::needless_pass_by_value)]with#[expect(...)]Implementation Details
floyd_find_cycle,brent_find_cycle) to avoid duplicationVerification
Ready for review.
Original prompt
Fixes #678
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit
New Features
Documentation
Tests
Refactor