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

Add convenience method Circuit.add_clexpr_from_logicexp() #1681

Merged
merged 4 commits into from
Nov 19, 2024
Merged

Conversation

cqc-alec
Copy link
Collaborator

@cqc-alec cqc-alec commented Nov 18, 2024

No description provided.

@cqc-alec cqc-alec changed the title Ae/clexprle Add convenience method Circuit.add_clexpr_from_logicexp() Nov 18, 2024
@cqc-alec cqc-alec marked this pull request as ready for review November 18, 2024 12:00
":param exp: logical expression\n"
":param output_bits: list of bits in output\n"
":return: the updated circuit",
py::arg("exp"), py::arg("output_bits"))
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
py::arg("exp"), py::arg("output_bits"))
py::arg("exp"), py::arg("output"))

Copy link
Contributor

Choose a reason for hiding this comment

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

And allow sequence[bits] and register?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I prefer output_bits as it is more explicit.

Any sequence of bits should work here.

In principle the output bits do not need to form a register (though that would be the case usually), so I'm not sure it's worth adding an overload.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the question is, would this work, and if not do we want it to?

c = Circuit()
x_reg = c.add_c_register('x', 3)
y_reg = c.add_c_register('y', 3)
z_reg = c.add_c_register('z', 3)
c.add_clexpr_from_logicexp(x_reg | y_reg, z_reg)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that does work; the register is coerced to a list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(ISTR mypy doesn't like it though.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add an additional stubs for that, to make mypy happy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure how to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would need a second c++ function in the binders for that, but not sure if there might be a easier way?
If you think that is not worth the effort happy to approve this as it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I don't think it's worth the effort (and maintenance) of an extra binder just to fix a mypy problem.

@cqc-alec cqc-alec requested a review from cqc-melf November 18, 2024 16:08
@cqc-alec cqc-alec merged commit b551702 into main Nov 19, 2024
30 checks passed
@cqc-alec cqc-alec deleted the ae/clexprle branch November 19, 2024 10:47
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