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!: Use CX count as default cost function #134

Merged
merged 4 commits into from
Sep 26, 2023
Merged

Conversation

lmondada
Copy link
Contributor

I realised testing #133 that ExhaustiveRewriteStrategy had its cost function (total gate count) hard coded. I changed that and used the opportunity to use CX count as the default cost function in both ExhaustiveRewriteStrategy and TASO.

@lmondada lmondada requested a review from ss2165 September 26, 2023 15:25
@lmondada lmondada changed the title feat[taso]: Use CX count as default cost function feat(taso)!: Use CX count as default cost function Sep 26, 2023
@lmondada lmondada changed the title feat(taso)!: Use CX count as default cost function feat!: Use CX count as default cost function Sep 26, 2023
@lmondada
Copy link
Contributor Author

lmondada commented Sep 26, 2023

I've implemented TryFrom<&OpType> and TryFrom<&LeafOp> for T2Op as the latter is Copy and we often only have references to OpTypes, but I'm not sure this is a proper use of the trait. Let me know if you can think of a better trait to use for this purpose.

Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines 126 to 133
pub fn exhaustive_cx() -> ExhaustiveRewriteStrategy<fn(&OpType) -> bool> {
ExhaustiveRewriteStrategy::with_predicate(is_cx)
}

/// Exhaustive rewrite strategy with CX count cost function and provided gamma.
pub fn exhaustive_cx_with_gamma(gamma: f64) -> ExhaustiveRewriteStrategy<fn(&OpType) -> bool> {
ExhaustiveRewriteStrategy::new(gamma, is_cx)
}
Copy link
Member

Choose a reason for hiding this comment

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

these would be nicer as associated functions of ExhaustiveRewriteStrategy

impl ExhaustiveRewriteStrategy<fn(&OpType) -> bool> {
    pub fn exhaustive_cx() -> Self {
        Self::with_predicate(is_cx)
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right, I did the same with default_with_eccs_json_file in TasoOptimiser.

@lmondada lmondada merged commit 969d463 into main Sep 26, 2023
7 checks passed
@lmondada lmondada deleted the feat/cx_cost_fn branch September 26, 2023 17:27
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.

2 participants