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.

Sign up for GitHub

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

Fix spurious PHASE and conditional in exponential_map #1182

Closed
wants to merge 1 commit into from
Closed

Fix spurious PHASE and conditional in exponential_map #1182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Changelog
- Fixed the QCS access request link in the README (@amyfbrown, gh-1171).
- Fix the SDK download link and instructions in the docs (@amyfbrown, gh-1173).
- Removed HALT from valid Protoquil / supported Quil. (@kilimanjaro, gh-1176).
- Fix spurious PHASE and conditional in exponential_map (@jlapeyre, gh-1182).
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a good sense for how likely it is that anyone depended on the old behavior. Do you think it's worth calling out in the changelog here that this is a potentially user visible / breaking change?

Likewise, should this go under the "Improvements and Changes" section, or is this more of a bugfix?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think

  • Worth calling out in the changelog. In addition to the (admittedly small) chance that users were explicitly relying on the old behavior, they could also be implicitly relying on it: qc.compiler.native_quil_to_executable(Program()) will fail with an error about an empty program.
  • Improvement, I think. The previous exponential_map, with the PHASE instructions etc, was correct in that the corresponding quantum operation was the identity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improvement

Agreed.

Worth calling out

That's a good point. There are two issues. The one about the empty program only concerns removing PHASE, not the inside-outside the closure one. The more I think about it, the more I think these should not both be changed in the same PR. Each one separately could break code.

Copy link
Contributor

Choose a reason for hiding this comment

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

The more I think about it, the more I think these should not both be changed in the same PR. Each one separately could break code.

This brings up the (somewhat painful now that we've been through multiple rounds of code review already :D) question of whether these changes are really worth it, since they might break user code, however unlikely.

I don't have a strong opinion either way. I agree it seems pretty unlikely anyone was depending on the old behavior, but you'd be surprised.

I also admit that I don't understand the use cases well enough to understand why the phase thing in the identity case is annoying / undesirable, so I defer to others on that one.

As for the inside-out closure change, I agree the new version is an improvement, but in the absence of any users complaining about it, I'd be inclined to let a sleeping dog lie. I also won't make a stink though if others feel like it's a minor enough breakage that it shouldn't matter... Maybe we should have a label for github issues that introduce breaking changes, to mark them for inclusion on some bright future day when we do a major version bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whether these changes are really worth it

Well, this is certainly not urgent, and it's somewhat contentious. The closure thing is just about code quality and clarity, not user experience.

phase thing in the identity case

AFAICT it was put there for some kind of testing or debugging purpose. It has no business being there in the production code. It is not even documented! You could debate like this though

Pro PHASE:
If the user asked for exponential_map of the identity, then we should not second guess the motive. We should return a function that does something non-trivial with it's parameter (phase angle) and still gives the correct behavior. Rotating back and forth is probably the simplest thing you can do. And, I guess do it on qubit zero, because... why put it on a different one ?

Anti PHASE:
exponential_map will be called on gates that were generated programmatically, not literally. The user would expect that it does the economic thing, eliding the operation on the identity. And certainly would expect that it does not do something arbitrary like play with the first qubit. Furthermore, if a nontrivial program is returned, It will later be elided by the compiler in any case.


[v2.17](https://github.com/rigetti/pyquil/compare/v2.16.0...v2.17.0) (January 30, 2020)
---------------------------------------------------------------------------------------
Expand Down
25 changes: 12 additions & 13 deletions pyquil/paulis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
)

from .quil import Program
from .gates import H, RZ, RX, CNOT, X, PHASE, QUANTUM_GATES
from .gates import H, RZ, RX, CNOT, QUANTUM_GATES
from numbers import Number
from collections import OrderedDict
import warnings
Expand Down Expand Up @@ -920,21 +920,20 @@ def exponential_map(term: PauliTerm) -> Callable[[float], Program]:
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: float) -> Program:
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:
if is_zero(term) or is_identity(term):

def exp_wrap(param: float) -> Program:
prog = Program()
return prog

else:

def exp_wrap(param: float) -> Program:
prog = Program()
prog += _exponentiate_general_case(term, param)
return prog
return prog

return exp_wrap

Expand Down
12 changes: 9 additions & 3 deletions pyquil/tests/test_paulis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import numpy as np
import pytest

from pyquil.gates import RX, RZ, CNOT, H, X, PHASE
from pyquil.gates import RX, RZ, CNOT, H
from pyquil.paulis import (
PauliTerm,
PauliSum,
Expand Down Expand Up @@ -421,15 +421,21 @@ def test_exponentiate_identity():
generator = PauliTerm("I", 1, 1.0)
para_prog = exponential_map(generator)
prog = para_prog(1)
result_prog = Program().inst([X(0), PHASE(-1.0, 0), X(0), PHASE(-1.0, 0)])
result_prog = Program()
assert prog == result_prog

generator = PauliTerm("I", 10, 0.08)
para_prog = exponential_map(generator)
prog = para_prog(1)
result_prog = Program().inst([X(0), PHASE(-0.08, 0), X(0), PHASE(-0.08, 0)])
result_prog = Program()
assert prog == result_prog

pop = 0.0 * sX(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: since this term is no longer sI, maybe move this out of test_exponentiate_identity and into a new test case called test_exponentiate_zero_coefficient or some such.

Also, a short comment explaining the "why" of this test would be nice. If someone ever changes the implementation and this test breaks, they'll be left wondering whether it's ok to delete the test, etc. Just a short note like "This is testing an implementation detail. Users are not expected to do this. blah blah blah."

Copy link
Contributor Author

@jlapeyre jlapeyre Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Re: the section. Oh yeah, I was tired, will fix that.

If someone ever changes

Great point. I'll add a comment.

expprog = exponential_map(pop)
assert expprog(0.5) == Program()
pop.coefficient = 1.0
assert expprog(0.5) == Program()


def test_trotterize():
term_one = PauliTerm("X", 0, 1.0)
Expand Down
10 changes: 5 additions & 5 deletions pyquil/tests/test_paulis_with_placeholders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import numpy as np
import pytest

from pyquil.gates import RX, RZ, CNOT, H, X, PHASE
from pyquil.gates import RX, RZ, CNOT, H
from pyquil.paulis import (
PauliTerm,
PauliSum,
Expand Down Expand Up @@ -398,14 +398,14 @@ def test_exponentiate_identity():
generator = PauliTerm("I", q[1], 1.0)
para_prog = exponential_map(generator)
prog = para_prog(1)
result_prog = Program().inst([X(q[0]), PHASE(-1.0, q[0]), X(q[0]), PHASE(-1.0, q[0])])
assert address_qubits(prog) == address_qubits(result_prog)
result_prog = Program()
assert prog == result_prog

generator = PauliTerm("I", q[10], 0.08)
para_prog = exponential_map(generator)
prog = para_prog(1)
result_prog = Program().inst([X(q[0]), PHASE(-0.08, q[0]), X(q[0]), PHASE(-0.08, q[0])])
assert address_qubits(prog) == address_qubits(result_prog)
result_prog = Program()
assert prog == result_prog


def test_trotterize():
Expand Down