exponential_map checks values of closed-over variable inside closure
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.
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
We could create a deep copy term, which would effectively make it immutable.
- The closed-over
PauliTermcontains mutable data. - If the data in
PauliTermis 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. - A user could rely on mutating the
PauliTermafter creating the closure. In that case, moving the conditional checks outside the creation of the closure could break the user's code. - It's very unlikely that the scenario in 3 is actually happening. (I think @ecpeterson said this, and I guess it is correct.)
- Closing over a copy of the
PauliTermand 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.
This should be addressed together with #373 and #537