-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Add badgerv2_unstable feature, RewriteSpace and CommitFactory #928
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
base: main
Are you sure you want to change the base?
Conversation
a5d0a42 to
c288a39
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## lm/hugr-v21 #928 +/- ##
==============================================
Coverage ? 76.61%
==============================================
Files ? 98
Lines ? 11709
Branches ? 11435
==============================================
Hits ? 8971
Misses ? 2119
Partials ? 619
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:
|
c288a39 to
d77a0c2
Compare
f881e07 to
00ba8e0
Compare
This replaces `TypeArg::Tuple` with `TypeArg::List` in the construction of a function type. It also changes the `Cargo.toml` to use the main branch of `hugr` so that Quantinuum/hugr#2378 is integrated, fixing more test failures. There's now one remaining test failure, related to Quantinuum/hugr#2387. It's unclear to me whether that is to be fixed in `tket2` or in `hugr`.
158ff4e to
7f5eb85
Compare
|
After trying out a lot of approaches, I've chosen to pull the This required changes to the z3-sys pull request: prove-rs/z3.rs#352 |
7f5eb85 to
a874669
Compare
a874669 to
6076d24
Compare
2db27ab to
0eca259
Compare
aborgna-q
left a comment
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.
Adding some comments, I still need to read through rewrite_space/
| hugr = { git = "https://github.com/CQCL/hugr", rev = "544759f4f472ddc4620c6679e11fe7551b46624c" } | ||
| hugr-core = { git = "https://github.com/CQCL/hugr", rev = "544759f4f472ddc4620c6679e11fe7551b46624c" } | ||
| hugr-cli = { git = "https://github.com/CQCL/hugr", rev = "544759f4f472ddc4620c6679e11fe7551b46624c" } |
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.
No need to set the "patch" above if setting a rev here.
Will this work with hugr 0.21 once released?
|
|
||
| [build-dependencies] | ||
| cc = "1.2" | ||
| cc = "1.1" |
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.
drive-by: Can you move the explicit dependency versions in this file to the workspace toml?
| [dependencies.z3] # Currently using a fork of z3.rs to install from GH releases | ||
| git = "https://github.com/lmondada/z3.rs.git" | ||
| rev = "d1ca853678c" | ||
| features = ["gh-release"] | ||
| optional = true |
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.
Merging this will prevent us from publishing tket2 releases.
Should we wait for your change to be published upstream?
| /// TKET2 extensions. | ||
| #[serde_as] | ||
| #[derive(Debug, Clone, From, Into, serde::Serialize, serde::Deserialize)] | ||
| pub struct HugrWithExts(#[serde_as(as = "AsStringTk2Envelope")] Hugr); |
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.
Maybe add a doc comment saying that this is not intendeed to be used directly, in case someone tries to use it as a HugrView?
| // DAN: If you need to produce serialised HUGRs, you can use this test | ||
| // and run it with `cargo test dummy_test_to_save_hugr`. |
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.
Not sure if we want to keep this comment :)
| for children_pair in children.combinations(2) { | ||
| let &[c1, c2] = children_pair.as_slice() else { | ||
| panic!("Expected 2 children, got {}", children_pair.len()); | ||
| }; |
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.
| for children_pair in children.combinations(2) { | |
| let &[c1, c2] = children_pair.as_slice() else { | |
| panic!("Expected 2 children, got {}", children_pair.len()); | |
| }; | |
| for &[c1, c2] in children.array_combinations() { |
| for children_pair in children.combinations(2) { | ||
| let &[c1, c2] = children_pair.as_slice() else { | ||
| panic!("Expected 2 children, got {}", children_pair.len()); | ||
| }; |
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.
How much do we expect the set of children to grow? Is it worth trying to reduce the quadratic cost?
(there's some tradeoffs we could take vs. the size of the invalidation sets)
As a starter, we should probably take the invalidation_set(c1, commit_id).collect()s out of the double-loop, and using BTreeSet::is_disjoint inside.
This requires Quantinuum/hugr#2349 and Quantinuum/hugr#2402 to be merged in, as well as a breaking hugr release before it can be merged.