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: Initialize Instructions from a Quil string. Python Instructions support the pickle module. #382

Merged
merged 6 commits into from
Jul 26, 2024

Conversation

MarquessV
Copy link
Contributor

@MarquessV MarquessV commented Jul 25, 2024

This is to support pyQuil#1791

todo:

  • Apply impl_pickle_for_instruction everywhere.

Copy link

github-actions bot commented Jul 25, 2024

PR Preview Action v1.4.7
🚀 Deployed preview to https://rigetti.github.io/quil-rs/pr-preview/pr-382/
on branch quil-py-docs at 2024-07-26 17:44 UTC

…e is leftover input, or if the parsed instruction count is not exactly 1
}
Ok(instructions.1[0].to_owned())
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we good with this being part of the public API?

@MarquessV MarquessV changed the title feat: Instruction types are compatible with the pickle module. feat: Initialize Instructions from a Quil string. Python Instructions support the pickle module. Jul 25, 2024
@@ -45,6 +45,66 @@ macro_rules! impl_to_quil {
};
}

/// Implements pickling for an instruction by implementing __getstate__ and __reduce__.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you link to the __reduce__ documentation below, but it would probably be better to link to that here as well, or even instead of doing it on __reduce__ itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the docstring per your comment.

Copy link
Contributor

@BatmanAoD BatmanAoD left a comment

Choose a reason for hiding this comment

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

I'm not familiar with this method of pickling, and I don't have an answer for your question about the public API (I don't know what harm it could do), but otherwise this looks good.

@jselig-rigetti
Copy link
Contributor

This is great! I see there's already a test for Program pickling at https://github.com/rigetti/quil-rs/blob/main/quil-py/test/program/test_program.py#L11-L23 does that use this now or is that doing something else?

@MarquessV
Copy link
Contributor Author

This is great! I see there's already a test for Program pickling at https://github.com/rigetti/quil-rs/blob/main/quil-py/test/program/test_program.py#L11-L23 does that use this now or is that doing something else?

Yeah, good eye. We already had a handwritten solution for Program previous to this. It hasn't changed with this PR, since the new macro is really built for the Instruction types. The implementation in the macro wouldn't be valid for Program.

@MarquessV MarquessV merged commit e500db8 into main Jul 26, 2024
14 checks passed
@MarquessV MarquessV deleted the implement-pickle-for-instructions branch July 26, 2024 18:36
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.

3 participants