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

fix(phirgen): pattern match on all possible regwise/bitwise ops #94

Merged
merged 5 commits into from
Jan 22, 2024

Conversation

qartik
Copy link
Member

@qartik qartik commented Jan 18, 2024

I need to confirm whether PECOS/PHIR supports the POW operation as the spec doesn't list it.

@qartik qartik linked an issue Jan 18, 2024 that may be closed by this pull request
cqc-alec
cqc-alec previously approved these changes Jan 18, 2024
@qartik qartik linked an issue Jan 18, 2024 that may be closed by this pull request
@qartik qartik changed the title fix(phirgen): pattern match on all possible regwise ops fix(phirgen): pattern match on all possible regwise/bitwise ops Jan 18, 2024
@qartik qartik marked this pull request as ready for review January 19, 2024 06:25
@qartik
Copy link
Member Author

qartik commented Jan 19, 2024

Marking as ready since the translation is correct. Currently PECOS doesn't support POW operation, but it can be added. If it's decided not to add support, it fails at PECOS.

@qartik qartik requested a review from cqc-alec January 19, 2024 06:26
@qartik qartik force-pushed the 93-missing-support-for-regwiseopand-and-regwiseopor branch from 0489eca to e4752f8 Compare January 19, 2024 07:14
@@ -118,23 +125,36 @@ def regwise_cop(exp: LogicExp) -> JsonDict:
cop = "<="
case RegWiseOp.GEQ:
cop = ">="
case RegWiseOp.NOT:
cop = "~"
case other:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why drop this error handler?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we are matching all possibilities, the other case is unreachable.

Copy link
Collaborator

@cqc-alec cqc-alec Jan 19, 2024

Choose a reason for hiding this comment

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

If we are matching all possibilities, the other case is unreachable.

Indeed, but is there any guarantee that we are? What if pytket were to add another one in the future (or the function were passed an invalid argument)? Wouldn't it be safer to keep the check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for making me think extra hard about this. Found a solution, just pushed.

@qartik qartik force-pushed the 93-missing-support-for-regwiseopand-and-regwiseopor branch from e4752f8 to facfb20 Compare January 20, 2024 07:22
@qartik qartik merged commit 7d63115 into main Jan 22, 2024
3 checks passed
@qartik qartik deleted the 93-missing-support-for-regwiseopand-and-regwiseopor branch January 22, 2024 11:19
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.

Missing support for RegWiseOp.AND and RegWiseOp.OR Bitwise operations not supported
2 participants