-
Notifications
You must be signed in to change notification settings - Fork 7
feat!: Add ReplaceBoolPass
#854
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
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #854 +/- ##
=======================================
Coverage 82.54% 82.54%
=======================================
Files 72 72
Lines 8861 8827 -34
Branches 8589 8555 -34
=======================================
- Hits 7314 7286 -28
+ Misses 1104 1098 -6
Partials 443 443
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:
|
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.
Nice!! Thanks @tatiana-s. A few comments, but my only real concern is in Cargo.toml
- presumably we will wait until the necessary Hugr stuff is in an actual release? (0.15.3 doesn't have array handlers but does have the necessary APIs, I think)
// (phasedx + 2*(float + load)) | ||
assert_eq!(h.node_count(), 59); | ||
// (phasedx + 2*(float + load)), tket2.read | ||
assert_eq!(h.node_count(), 60); |
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.
Cripes. I admit 59->60 is not a big change so I'm not saying this is a new problem ;-), but such a check does not give me much assurance. Could it perhaps be a Hugr "snapshot" (equality check) or something? (Maybe for another PR!)
@@ -386,8 +387,9 @@ pub trait QSystemOpBuilder: Dataflow + UnwrapBuilder { | |||
/// Build a projective measurement with a conditional flip. | |||
fn build_measure_flip(&mut self, qb: Wire) -> Result<[Wire; 2], BuildError> { | |||
let [qb, b] = self.add_measure_reset(qb)?; | |||
let sum_b = self.add_dataflow_op(BoolOp::read, [b]).unwrap().out_wire(0); |
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.
Nice! Great that this critical addition is so simple :)
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.
I note this is in QSystemOpBuilder - is the idea that for non-qsystem we use a completely different pathway (rather than, say, beginning like this and then mapping bool_type
to bool_t
and read
to identity)? I'm not sure whether the other even exists atm of course so this is probably the right place.
@@ -26,6 +26,7 @@ use hugr::{ | |||
use derive_more::Display; | |||
use lazy_static::lazy_static; | |||
use strum::{EnumIter, EnumString, IntoStaticStr}; | |||
use tket2::extension::bool::{bool_type, BoolOp}; |
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.
bool_type
is not new or even touched in this PR but I have to wonder whether bool_type
(opaque) vs bool_t
(Hugr prelude / Sum) is the best naming scheme ;-). Might this be a good time to consider renaming bool_type
to e.g. opaque_bool
(_t
), tket2_bool
, something like that?
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.
I agree, this is going to be annoying. I think my preference is opaque_bool_t
bool_to_sum, | ||
sum_to_bool, | ||
// Gets a Hugr bool_t value from the opaque type. | ||
read, |
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.
👍
tket2/src/extension/bool.rs
Outdated
/// Add a "tket2.bool.SumToBool" op. | ||
fn add_sum_to_bool(&mut self, sum_input: Wire) -> Result<[Wire; 1], BuildError> { | ||
/// Add a "tket2.bool.make_opaque" op. | ||
fn add_make_bool_opaque(&mut self, sum_input: Wire) -> Result<[Wire; 1], BuildError> { |
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.
by previous convention, add_bool_make_opaque
? Or add_read_bool
and add_make_opaque_bool
? Or maybe there is no convention ;)
tket2-hseries/src/replace_bools.rs
Outdated
/// An error reported from [ReplaceBoolPass]. | ||
pub enum ReplaceBoolPassError<N> { | ||
/// The HUGR was invalid either before or after a pass ran. | ||
ValidationError(ValidatePassError), |
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.
Side note, depending on whether this goes in before or after Hugr 0.16.0, a fair bit may change here according to CQCL/hugr#1895
tket2-hseries/src/replace_bools.rs
Outdated
|
||
// Replace tket2.bool type. | ||
lw.replace_type(bool_type().as_extension().unwrap().clone(), bool_dest()); | ||
let bool_arg = TypeArg::Type { |
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.
let bool_arg = TypeArg::Type { | |
let bool_arg: TypeArg = bool_t().clone().into(); |
tket2-hseries/src/replace_bools.rs
Outdated
ty: bool_t().clone(), | ||
}; | ||
let dup_op = ExtensionOp::new( | ||
futures::EXTENSION.get_op("Dup").unwrap().clone(), |
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.
add "Dup" and "Free" as constants inside futures
? (esp. if capitalization is important!)
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.
Hm I could use futures::FutureOpDef::Dup.name().as_str()
but then that wouldn't work again with CQCL/hugr#1496 - but adding constants seems strange when you should be able to get the name just from the definition
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.
HI agree, name
is not the right thing. However you can use https://docs.rs/strum/latest/strum/derive.IntoStaticStr.html which is impld for FutoreOpDef
I would prefer a different approach FutureOp { op: FutureOpDef::Dup, typ= todo!() }.to_extension_op().unwrap()
tket2-hseries/src/replace_bools.rs
Outdated
.unwrap(); | ||
|
||
// Replace all tket2.bool ops. | ||
let read_op = ExtensionOp::new(BOOL_EXTENSION.get_op("read").unwrap().clone(), vec![]).unwrap(); |
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.
Similarly make constants for read/make_opaque op names
tket2-hseries/src/replace_bools.rs
Outdated
|
||
// Replace measure ops with lazy versions. | ||
let tket2_measure_free = ExtensionOp::new( | ||
TKET2_EXTENSION.get_op("MeasureFree").unwrap().clone(), |
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.
And here
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.
This is great, thank you. I think you can remove all these string constants as I describe inline.
Registering all the ops with the linearizer looks ripe for a place one forgets to update when things change, even with the strings removed. Adding a new op to the bool extension, for example. I don't see a better way though.
@@ -26,6 +26,7 @@ use hugr::{ | |||
use derive_more::Display; | |||
use lazy_static::lazy_static; | |||
use strum::{EnumIter, EnumString, IntoStaticStr}; | |||
use tket2::extension::bool::{bool_type, BoolOp}; |
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.
I agree, this is going to be annoying. I think my preference is opaque_bool_t
@@ -386,8 +387,9 @@ pub trait QSystemOpBuilder: Dataflow + UnwrapBuilder { | |||
/// Build a projective measurement with a conditional flip. | |||
fn build_measure_flip(&mut self, qb: Wire) -> Result<[Wire; 2], BuildError> { | |||
let [qb, b] = self.add_measure_reset(qb)?; | |||
let sum_b = self.add_dataflow_op(BoolOp::read, [b]).unwrap().out_wire(0); |
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.
let sum_b = self.add_dataflow_op(BoolOp::read, [b]).unwrap().out_wire(0); | |
let sum_b = self.add_dataflow_op(BoolOp::read, [b])?.out_wire(0); |
tket2-hseries/src/replace_bools.rs
Outdated
ty: bool_t().clone(), | ||
}; | ||
let dup_op = ExtensionOp::new( | ||
futures::EXTENSION.get_op("Dup").unwrap().clone(), |
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.
HI agree, name
is not the right thing. However you can use https://docs.rs/strum/latest/strum/derive.IntoStaticStr.html which is impld for FutoreOpDef
I would prefer a different approach FutureOp { op: FutureOpDef::Dup, typ= todo!() }.to_extension_op().unwrap()
tket2-hseries/src/replace_bools.rs
Outdated
) | ||
.unwrap(); | ||
lw.replace_op(&make_opaque_op, make_opaque_op_dest()); | ||
for op_name in ["eq", "and", "or", "xor"] { |
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.
This is pretty gross! I think you can do `[BoolOp::or, BoolOp::eq,] etc
tket2-hseries/src/replace_bools.rs
Outdated
"xor" => dfb | ||
.add_dataflow_op(LogicOp::Xor, [cond1.out_wire(0), cond2.out_wire(0)]) | ||
.unwrap(), | ||
_ => panic!("Unknown op name"), |
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.
_ => panic!("Unknown op name"), | |
op => panic!("Unknown op name: {op}"), |
99b7c97
to
7dc78b2
Compare
Closes #867 drive-by do a cargo update
will break on subsequent bump when these methods are removed
🤖 I have created a release *beep* *boop* --- ## [0.8.0](tket2-exts-v0.7.0...tket2-exts-v0.8.0) (2025-05-22) ### ⚠ BREAKING CHANGES * (`tket2.bool` extension) `BoolOp::bool_to_sum` / `BoolOp::sum_to_bool` renamed to `BoolOp::read` / `BoolOp::make_opaque` `Tk2Op::MeasureFree` now returns a `tket2.bool` (`tket2-hseries.qsystem` extension) `QSystemOp:Measure` and `QSystemOp:MeasureReset` now return `tket2.bool`s * **tket2-hseries:** `QSystemOpBuilder` gained supertrait `ArrayOpBuilder` ### Features * Add `ReplaceBoolPass` ([#854](#854)) ([5ae0ab9](5ae0ab9)) * **tket2-hseries:** insert RuntimeBarrier across qubits in a Barrier ([#866](#866)) ([6bcc9d6](6bcc9d6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Seyon Sivarajah <[email protected]>
## 🤖 New release * `tket2`: 0.10.0 -> 0.11.0 (✓ API compatible changes) * `tket2-hseries`: 0.13.0 -> 0.14.0 (⚠ API breaking changes) ### ⚠ `tket2-hseries` breaking changes ```text --- failure enum_missing: pub enum removed or renamed --- Description: A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/enum_missing.ron Failed in: enum tket2_hseries::lazify_measure::LazifyMeasurePassError, previously in file /tmp/.tmpd3CB3X/tket2-hseries/src/lazify_measure.rs:63 enum tket2_hseries::lazify_measure::LazifyMeasureRewrite, previously in file /tmp/.tmpd3CB3X/tket2-hseries/src/lazify_measure.rs:110 --- failure enum_variant_missing: pub enum variant removed or renamed --- Description: A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/enum_variant_missing.ron Failed in: variant QSystemPassError::LazyMeasureError, previously in file /tmp/.tmpd3CB3X/tket2-hseries/src/lib.rs:58 --- failure function_missing: pub fn removed or renamed --- Description: A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/function_missing.ron Failed in: function tket2_hseries::lazify_measure::replace_measure_ops, previously in file /tmp/.tmpd3CB3X/tket2-hseries/src/lazify_measure.rs:79 --- failure module_missing: pub module removed or renamed --- Description: A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/module_missing.ron Failed in: mod tket2_hseries::lazify_measure, previously in file /tmp/.tmpd3CB3X/tket2-hseries/src/lazify_measure.rs:1 --- failure struct_missing: pub struct removed or renamed --- Description: A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/struct_missing.ron Failed in: struct tket2_hseries::lazify_measure::LazifyMeasurePass, previously in file /tmp/.tmpd3CB3X/tket2-hseries/src/lazify_measure.rs:46 --- failure trait_added_supertrait: non-sealed trait added new supertraits --- Description: A non-sealed trait added one or more supertraits, which breaks downstream implementations of the trait ref: https://doc.rust-lang.org/cargo/reference/semver.html#generic-bounds-tighten impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_added_supertrait.ron Failed in: trait tket2_hseries::extension::qsystem::QSystemOpBuilder gained ArrayOpBuilder in file /tmp/.tmpYKdcGW/tket2/tket2-hseries/src/extension/qsystem.rs:222 ``` <details><summary><i><b>Changelog</b></i></summary><p> ## `tket2` <blockquote> ## [0.11.0](tket2-v0.10.0...tket2-v0.11.0) - 2025-05-22 ### New Features - [**breaking**] Add `ReplaceBoolPass` ([#854](#854)) ### Refactor - Use black_box from standard library. ([#878](#878)) </blockquote> ## `tket2-hseries` <blockquote> ## [0.14.0](tket2-hseries-v0.13.0...tket2-hseries-v0.14.0) - 2025-05-22 ### Bug Fixes - *(tket2-hseries)* ensure deterministic lowering using maps ([#884](#884)) ### New Features - *(tket2-hseries)* [**breaking**] insert RuntimeBarrier across qubits in a Barrier ([#866](#866)) - [**breaking**] Add `ReplaceBoolPass` ([#854](#854)) - *(tket2-hseries)* Remove `static_array<tket2.bool>` before `replace_bool`ing. ([#885](#885)) ### Refactor - *(tket2-hseries)* use smaller angle decompositions for CZ and CCX ([#883](#883)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). --------- Co-authored-by: Seyon Sivarajah <[email protected]>
To replace
LazifyMeasurePass
, see https://github.com/quantinuum-dev/hugrverse/issues/130Closes #857
BREAKING CHANGE: (
tket2.bool
extension)BoolOp::bool_to_sum
/BoolOp::sum_to_bool
renamed toBoolOp::read
/BoolOp::make_opaque
BREAKING CHANGE:
Tk2Op::MeasureFree
now returns atket2.bool
BREAKING CHANGE: (
tket2-hseries.qsystem
extension)QSystemOp:Measure
andQSystemOp:MeasureReset
now returntket2.bool
s