Skip to content
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

feat: Add tket2.bool extension #823

Merged
merged 15 commits into from
Mar 26, 2025
Merged

feat: Add tket2.bool extension #823

merged 15 commits into from
Mar 26, 2025

Conversation

tatiana-s
Copy link
Contributor

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 76.83616% with 41 lines in your changes missing coverage. Please review.

Project coverage is 82.46%. Comparing base (9b31135) to head (ef8f8aa).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
tket2/src/extension/bool.rs 77.64% 36 Missing and 2 partials ⚠️
tket2-exts/src/tket2_exts/__init__.py 66.66% 2 Missing ⚠️
tket2-hseries/src/bin/tket2-hseries.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #823      +/-   ##
==========================================
- Coverage   82.58%   82.46%   -0.13%     
==========================================
  Files          65       66       +1     
  Lines        8097     8274     +177     
  Branches     7834     8005     +171     
==========================================
+ Hits         6687     6823     +136     
- Misses       1005     1044      +39     
- Partials      405      407       +2     
Flag Coverage Δ
python 81.78% <66.66%> (-0.35%) ⬇️
rust 82.48% <77.19%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tatiana-s tatiana-s requested a review from doug-q March 20, 2025 10:18
@tatiana-s tatiana-s marked this pull request as ready for review March 20, 2025 10:18
@tatiana-s tatiana-s requested a review from a team as a code owner March 20, 2025 10:18
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explain the reason why this exists in addition to sums.

Is this only meant as a guppy artifact? The documentation seems to imply so.

Copy link
Contributor

@doug-q doug-q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you! I thought we were going to add this to the tket2 crate rather than tket2-hseries? I would put it next to rotation.

I don't think we should reference guppy in the definition because there's nothing guppy-specific here.

@@ -0,0 +1,384 @@
//! This module defines the Hugr extension used to represent bools in Guppy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! This module defines the Hugr extension used to represent bools in Guppy.
//! This module defines the Hugr extension used to represent bools as an opaque type.

ext.add_type(
BOOL_TYPE_NAME.to_owned(),
vec![],
"The Guppy bool type".into(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"The Guppy bool type".into(),
"An opaque bool type".into(),

)
}

/// Returns a `bool` [Type].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Returns a `bool` [Type].
/// Returns a `tket2.bool` [Type].

}

#[derive(Debug, Clone, PartialEq, Hash, serde::Serialize, serde::Deserialize)]
/// Structure for holding constant bool values.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Structure for holding constant bool values.
/// Structure for holding constant `tket2.bool` values.

Comment on lines 81 to 83
pub fn value(&self) -> &bool {
&self.0
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn value(&self) -> &bool {
&self.0
}
pub fn value(&self) -> bool {
self.0
}

bool impls Copy, no point returnin a reference I don't think.

#[typetag::serde]
impl CustomConst for ConstBool {
fn name(&self) -> ValueName {
format!("ConstBool({:?})", self.0).into()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
format!("ConstBool({:?})", self.0).into()
format!("ConstBool({self.0})").into()

Note that this removes the :?, so we are using Display over Debug to convert to a string. Usually that's what you want.

#[non_exhaustive]
/// Simple enum of "tket2.bool" operations.
pub enum BoolOpDef {
BoolToSum,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we use snake case constructors for this, i.e. bool_to_sum. because these are user-facing names


fn description(&self) -> String {
match self {
BoolOpDef::BoolToSum => "Convert a Guppy bool into a Hugr unit sum.".into(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
BoolOpDef::BoolToSum => "Convert a Guppy bool into a Hugr unit sum.".into(),
BoolOpDef::BoolToSum => "Convert a tket2.bool into a Hugr unit sum.".into(),

And the same for below


#[derive(Debug, Clone, PartialEq, Eq, Hash)]
/// Concrete instances of a "tket2.bool" operations.
pub enum BoolOp {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need for both BoolOp and BoolOpDef here, because these ops have no type parameters. See QSystemOp for an example of this case(although it does not follow my advice above of snake_case_names! awkward)

@tatiana-s tatiana-s requested a review from doug-q March 20, 2025 11:28
Copy link
Contributor

@doug-q doug-q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! You could improve your coverage by adding a test that exercises all of the eq/or/not etc ops

@tatiana-s tatiana-s added this pull request to the merge queue Mar 26, 2025
Merged via the queue into main with commit 8818d2f Mar 26, 2025
17 of 18 checks passed
@tatiana-s tatiana-s deleted the ts/guppy-bool branch March 26, 2025 15:30
This was referenced Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add opaque bool extension to tket2
3 participants