pyquil icon indicating copy to clipboard operation
pyquil copied to clipboard

exponential_map checks values of closed-over variable inside closure

Open jlapeyre opened this issue 6 years ago • 4 comments

Pre-Report Checklist

  • [x] I am running the latest versions of pyQuil and the Forest SDK
  • [x] I checked to make sure that this bug has not already been reported

Issue Description

Insert a short description of the bug here, along with what you expected the behavior to be.

Thanks for helping us improve pyQuil! 🙂

How to Reproduce

If useful, provide a numbered list of the steps that result in the error.

Otherwise, just fill out the "Code Snippet" and "Error Output" sections below.

Code Snippet

Include a snippet of the pyQuil code that produces the error here.

Error Output

Additionally, please copy and paste the output of the above code here.

Environment Context

Operating System:

Python Version (python -V):

Quilc Version (quilc --version):

QVM Version (qvm --version):

Python Environment Details (pip freeze or conda list):

Copy and paste the output of `pip freeze` or `conda list` here.

jlapeyre avatar Oct 16 '19 21:10 jlapeyre

exponential_map takes a parameter and returns a function closing over that parameter. But, the returned function includes conditionals testing the value of the parameter. These conditionals should probably be moved outside the body of the returned function. OTOH, as @appleby mentioned in another channel, the value that is closed over includes mutable attributes, so someone might be relying on this behavior.

https://github.com/rigetti/pyquil/blob/527138bd9e87190f452ffb2c46572b343cb35ba9/pyquil/paulis.py#L837-L864

jlapeyre avatar Oct 16 '19 22:10 jlapeyre

We could create a deep copy term, which would effectively make it immutable.

jlapeyre avatar Oct 16 '19 22:10 jlapeyre

  1. The closed-over PauliTerm contains mutable data.
  2. If the data in PauliTerm is in fact not changed after the creation of the closure, then the conditional checks inside the closure are redundant. But, the inefficiency introduced is probably negligible.
  3. A user could rely on mutating the PauliTerm after creating the closure. In that case, moving the conditional checks outside the creation of the closure could break the user's code.
  4. It's very unlikely that the scenario in 3 is actually happening. (I think @ecpeterson said this, and I guess it is correct.)
  5. Closing over a copy of the PauliTerm and moving the conditionals outside of the function creation would make the intent more clear. There is a small chance it would break someone's code. But, that would likely be revealing a bug in that code.

jlapeyre avatar Feb 24 '20 22:02 jlapeyre

This should be addressed together with #373 and #537

jlapeyre avatar Feb 24 '20 22:02 jlapeyre