exponential_map checks values of closed-over variable inside closure

[jlapeyre]

Pre-Report Checklist

• I am running the latest versions of pyQuil and the Forest SDK
• 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]

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.

pyquil/pyquil/paulis.py

Lines 837 to 864 in 527138b

	def exponential_map(term):
	"""
	Returns a function f(alpha) that constructs the Program corresponding to exp(-1j*alpha*term).
	:param term: A pauli term to exponentiate
	:returns: A function that takes an angle parameter and returns a program.
	:rtype: Function
	"""
	if not np.isclose(np.imag(term.coefficient), 0.0):
	raise TypeError("PauliTerm coefficient must be real")
	coeff = term.coefficient.real
	term.coefficient = term.coefficient.real
	def exp_wrap(param):
	prog = Program()
	if is_identity(term):
	prog.inst(X(0))
	prog.inst(PHASE(-param * coeff, 0))
	prog.inst(X(0))
	prog.inst(PHASE(-param * coeff, 0))
	elif is_zero(term):
	pass
	else:
	prog += _exponentiate_general_case(term, param)
	return prog
	return exp_wrap

[jlapeyre]

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

[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]

This should be addressed together with #373 and #537