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

Use pickle __reduce__ for TwoQubitWeylDecomposition #12003

Merged
merged 1 commit into from
Mar 16, 2024

Conversation

jakelishman
Copy link
Member

Summary

Since this class does complicated calculation within its __new__ method (as many extension classes do), we want to use an alternative extension-module constructor for the object on unpickle, to avoid leaking the "pickle" / "no pickle" state through the default constructor.

This implements a private Python-space method that directly constructs the object from components, which is then used as the __reduce__ target, along with the pickle state.

Details and comments

Built on top of #11946, since that might be on the verge of landing. See #11946 (comment) for context.

It's plausible we might want to expose _from_state as a public constructor, or otherwise provide a convenient debugging method that produces a copy/paste codeblock in terms of it, since unlike from_bytes, it completely skips all the recalculation of the decomposition and simply builds the state in place; there's zero risk of floating-point errors from the re-decomposition, because it simply doesn't happen.

@jakelishman jakelishman added on hold Can not fix yet Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository mod: transpiler Issues and PRs related to Transpiler labels Mar 13, 2024
@jakelishman jakelishman added this to the 1.1.0 milestone Mar 13, 2024
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Eric-Arellano
  • @Qiskit/terra-core
  • @kevinhartman
  • @levbishop
  • @mtreinish

@coveralls
Copy link

coveralls commented Mar 13, 2024

Pull Request Test Coverage Report for Build 8283866556

Details

  • 76 of 92 (82.61%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.008%) to 89.258%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/two_qubit_decompose.rs 66 70 94.29%
crates/accelerate/src/euler_one_qubit_decomposer.rs 10 22 45.45%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/euler_one_qubit_decomposer.rs 1 91.05%
crates/qasm2/src/lex.rs 4 91.69%
Totals Coverage Status
Change from base Build 8282173373: 0.008%
Covered Lines: 59547
Relevant Lines: 66713

💛 - Coveralls

@levbishop
Copy link
Member

Looks like a better way to do things for pickle.

Not sure that I agree for exposing this to from_bytes: although a discrepancy between the original and reconstructed state does indicate a floating point error, it may equally likely be an error in the initial decomposition giving a chance to fix it at the re-decomposition? Neither seems obviously better to me, and at least with the current re-decomposition the numerical stability gets checked in the test suite.

@jakelishman
Copy link
Member Author

We can check numerical stability by just trying to do the same decomposition more than once, right? TwoQubitWeylDecomposition(u) == TwoQubitWeylDecomposition(u) would be a more thorough attempted check of that, than the from_bytes method that requires quite a lot of separate code to maintain to make it work. That's got easier with the Rust-space version of things, since now we don't have to dance through __new__ quite so heavily to handle subclassing, but I'd argue that our debugging tool not being a direct bitcopy reproduction is actually really painful. Checking for numerical stability is a good thing for the test suite to do for sure, but having it be a required part of reconstructing the object at all is not great for debugging, I think.

Since this class does complicated calculation within its `__new__`
method (as many extension classes do), we want to use an alternative
extension-module constructor for the object on unpickle, to avoid
leaking the "pickle" / "no pickle" state through the default
constructor.

This implements a private Python-space method that directly constructs
the object from components, which is then used as the `__reduce__`
target, along with the pickle state.
@jakelishman
Copy link
Member Author

Now rebased over main, as #11946 merged.

@jakelishman jakelishman removed the on hold Can not fix yet label Mar 14, 2024
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, this is a cleaner solution to handle pickling without having to worry about duplicate calculations.

@mtreinish mtreinish added this pull request to the merge queue Mar 15, 2024
@mtreinish mtreinish removed this pull request from the merge queue due to a manual request Mar 15, 2024
@mtreinish mtreinish added this pull request to the merge queue Mar 16, 2024
Merged via the queue into Qiskit:main with commit 6e280c1 Mar 16, 2024
12 checks passed
@jakelishman jakelishman deleted the weyl-pickle branch March 16, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog mod: transpiler Issues and PRs related to Transpiler Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants