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

Back Frame and DefFrame with quil_rs #1542

Conversation

MarquessV
Copy link
Contributor

Description

Does what it says on the tin, backs the Frame and DefFrame classes with quil_rs.FrameIdentifier, and quil_rs.FrameDefinition, respectively.

The original classes were @dataclasses. In order to subclass them properly, I removed the @dataclass decorator. This may result in unexpected breaking changes. Might be worth labeling it as such, even if we think most of the functionality is the same.

pyquil/quilatom.py Outdated Show resolved Hide resolved
test/unit/test_quilatom.py Show resolved Hide resolved
qubits: Tuple[Union[Qubit, FormalArgument], ...]
""" A tuple of qubits on which the frame exists. """
@staticmethod
def __new__(cls, qubits: Sequence[Union[int, Qubit, FormalArgument]], name: str) -> "Frame":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like name should be the first parameter here for consistency with the other instructions. Should we add a deprecation warning?

Copy link
Contributor

@kalzoo kalzoo Mar 16, 2023

Choose a reason for hiding this comment

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

What do you think about making it keyword-only so it doesn't matter? I suppose that might be inconsistent with other constructors, but it's more future-proof than just switching them and is good practice anyway

It may also make for a more useful error for scripts where they are present and out of order, than just a type error when qubits are used where a name is expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do like that idea, do we want to commit to that for all instructions? How do we best warn users about that future change? So far we've been adding deprecation warnings, but that would be pretty noisy if all instructions had one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this discussion seems like it'll have impact beyond just this instruction, I've made a note to have a followup discussion and will push forward with this PR as is.

test/unit/test_quilbase.py Show resolved Hide resolved
@MarquessV MarquessV marked this pull request as ready for review March 16, 2023 16:39
@MarquessV MarquessV requested a review from a team as a code owner March 16, 2023 16:39
Copy link
Contributor

@kalzoo kalzoo left a comment

Choose a reason for hiding this comment

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

The original classes were @dataclasses. In order to subclass them properly, I removed the @DataClass decorator. This may result in unexpected breaking changes. Might be worth labeling it as such, even if we think most of the functionality is the same.

Agreed.

Reviewed for anything that looked out of place - did not see anything. There may be omitted/missing behavior but we'll spot that in e2e testing if that's the case

qubits: Tuple[Union[Qubit, FormalArgument], ...]
""" A tuple of qubits on which the frame exists. """
@staticmethod
def __new__(cls, qubits: Sequence[Union[int, Qubit, FormalArgument]], name: str) -> "Frame":
Copy link
Contributor

@kalzoo kalzoo Mar 16, 2023

Choose a reason for hiding this comment

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

What do you think about making it keyword-only so it doesn't matter? I suppose that might be inconsistent with other constructors, but it's more future-proof than just switching them and is good practice anyway

It may also make for a more useful error for scripts where they are present and out of order, than just a type error when qubits are used where a name is expected.

pyquil/quilatom.py Outdated Show resolved Hide resolved
pyquil/quilatom.py Outdated Show resolved Hide resolved
test/unit/test_quilbase.py Show resolved Hide resolved
@MarquessV MarquessV merged commit 961a8d2 into v4-feature-program-implemented-via-qcs-sdk-rust Mar 17, 2023
@MarquessV MarquessV deleted the v4-feature-program-use-defframe-api branch March 17, 2023 01:31
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