Integrate petgraph#28
Merged
Merged
Conversation
daehiff
commented
Mar 31, 2025
daehiff
commented
Mar 31, 2025
daehiff
commented
Mar 31, 2025
daehiff
commented
Apr 1, 2025
daehiff
commented
Apr 2, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR integrates petgraph into the project by removing legacy architectures (Line and Complete) and consolidating functionality into a new Connectivity implementation that leverages petgraph’s algorithms. Key changes include:
- Removal of src/architecture/line.rs and src/architecture/complete.rs.
- Implementation and enhancement of Connectivity with new functions for line, grid, and complete architectures as well as CX ladder extraction.
- Updates to tests, benchmarks, and Cargo.toml to support petgraph and related dependencies.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/architecture/line.rs | Entirely removed to consolidate architecture implementations into Connectivity. |
| src/architecture/connectivity.rs | Added petgraph integration, new connectivity constructors (line, grid, complete), and CX ladder extraction API. |
| src/architecture/complete.rs | Removed as its functionality is replaced by Connectivity. |
| src/architecture.rs | Updated to include the new LadderError enum and an extended Architecture trait. |
| benches/connectivity.rs | Added benchmarks for new connectivity functionality. |
| benches/clifford_tableau.rs | Updated benchmark configuration to include connectivity benchmarks. |
| Cargo.toml | Adjusted petgraph dependency revision and added the rand dependency. |
Comments suppressed due to low confidence (3)
src/architecture/connectivity.rs:326
- Using .sort() inline in the assertion causes both sides to evaluate to () since .sort() sorts in place. Consider assigning the result to a mutable variable, sorting it, and then comparing the sorted vectors.
.unwrap().sort(),
src/architecture/connectivity.rs:367
- Calling .sort() here returns unit; this could lead to improper test comparisons. Refactor the test to sort the returned vector separately before the assertion.
.unwrap().sort(),
src/architecture/connectivity.rs:378
- The inline use of .sort() results in a unit value, which makes the comparison invalid. Store the ladder results in a mutable variable, sort them, and then perform the equality check.
.unwrap().sort(),
c5ce2fc to
59b7436
Compare
There was a problem hiding this comment.
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/architecture/connectivity.rs:316
- The use of .sort() directly in the assert_eq! call is problematic because .sort() returns () instead of the sorted vector. Consider assigning the ladder outputs to mutable variables, sorting them, and then comparing the sorted vectors.
new_architecture.get_cx_ladder(&vec![1, 2, 3, 4], &2).unwrap().sort(),
daehiff
commented
Apr 8, 2025
daehiff
commented
Apr 8, 2025
daehiff
commented
Apr 8, 2025
Aerylia
reviewed
Apr 8, 2025
Aerylia
left a comment
Contributor
There was a problem hiding this comment.
Left some minor comments regarding function signatures end tests
04eff6c to
3ebed56
Compare
Signed-off-by: daehiff <winderl13@gmail.com>
3ebed56 to
8031626
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.