Fix spurious PHASE and conditional in exponential_map

[jlapeyre]

• exponential_map of the identity applies a PHASE gate and
  its inverse. This was apparently for debugging purposes. This
  PR replaces the PHASE gates with an empty Program.

• This PR moves conditionals on data that is closed over from
  inside the closure to outside. That is, the gate that is exponentiated
  is checked when the closure is created, but not when it is executed.

Closes #1055. This PR obsoletes PR #373.

Checklist

• [ x] The above description motivates these changes.
• [ x] There is a unit test that covers these changes.
• [x ] All new and existing tests pass locally and on [Travis CI][travis].
• [x ] (Bugfix) The associated issue is referenced above using [auto-close keywords][auto-close].
• [ x] The [changelog][changelog] is updated, including author and PR number (@username, gh-xxx).

[appleby]

This feels a little fragile. Would it be reasonable to replace this with a similar test that instead exercises the is_zero case? Then you'd only have to modify pop.coefficient here no? (which seems preferable since coefficient is at least a "public" attribute). I guess that would no longer make this an identity test, but it could be moved into a new test case or even test_exponentiate_prog above.

[jlapeyre]

Just to be super explicit: It looks like you see that I want a test that would have failed before this PR, and you think this is a reasonable want. I like the idea of using is_zero. I'm not super familiar with how APIs are determined in python. But, you don't need a degree in pythonology to see that pop.__dict__["_ops"][0] is not in the API. I guess a data field (attribute) like coefficient is in by default ? It's certainly more API'ish.

What I most don't like about this test is that the rest of the file tests the API. Testing implementation that is not part of the API should, if it's desirable at all, go elsewhere. The point of the test is anticipating someone doing something they shouldn't be doing anyway.

[appleby]

The convention in python is that any attribute prefixed with one (or more) underscores is considered a "private" implementation detail, while attributes without a leading _ are part of the "public" API. (Double underscores on either side are a special case that are reserved for Python).

In general I'm not opposed to adding tests that document an implementation detail, but I'd also be fine in this case if we just dropped the test or else switched to testing the is_zero case since (I think) we could then stick to accessing the "public" API.

[jlapeyre]

Regarding underscores. Just a question, don't some people go further and demand the use of accessors for public attributes ?

It's not clear to me why coefficient has been made public and the operators private. My instinct is that PauliTerms should be immutable (if only by explicit convention). But, that's a another issue. And it's much more likely that people have been mutating coefficient.

[appleby]

Using explicit accessors like get_foo is rare in python, mostly because Python has a concept of Descriptors and "properties" that let you transparently wrap attribute lookup and assignment in a call to a function getter / setter.

Conventions certainly differ, but generally speaking any attribute that doesn't start with a leading underscore is considered "public". As far as I know, we follow this convention in pyquil.

As to why the coefficient attribute was made "public", I couldn't say. It's possible that it was just an oversight and never intended to be mutated, but given that it's already "public", we're stuck with it so to speak.

[jlapeyre]

EDIT: Sorry I did not reload this page so I did not see your comment above before writing the following comment.

I somehow forgot that pop.__dict__["_ops"][0] is not necessary. pop._ops[0] will do. (Hmm pop is not a good variable name, maybe term for consistency with the other tests, or pauli_term). Still _ops has one underscore and coefficient does not. However, reviewing the class PauliTerm it seems very likely that PauliTerm objects are meant to be immutable. Unfortunately, this is not stated explicitly. I think the underscore is there because the class provides interfaces the operators that are no-so tied to the implementation, whereas coefficient is straightforward. Another piece of evidence: The class function term_with_coeff says it is to "Change the coefficient of a PauliTerm". But, what it actually does is return a copy with a different coefficient.

The issue of mutability is directly relevant here, because it determines whether we are breaking the API, even a little bit. What to do ? I think continuing with this PR and making a note in the changelog, as you suggest is the best course. But, we should come to a consensus on whether PauliTerm is mutable. WDYT ?

[appleby]

Hmm pop is not a good variable name, maybe term for consistency with the other tests

I almost made this comment, but didn't want to risk NITPICK OVERLOAD :)

The issue of mutability is directly relevant here, because it determines whether we are breaking the API, even a little bit. What to do ? I think continuing with this PR and making a note in the changelog, as you suggest is the best course. But, we should come to a consensus on whether PauliTerm is mutable. WDYT ?

I think the fact that the coefficient attribute is "public" means that PauliTerm is technically mutable, regardless of what the original intention was or whether we like it now. We could change the API to enforce immutability (or at least more strongly discourage mutability), but that seems like a bigger breaking change and something that should only be done in a major version bump.

[jlapeyre]

nitpick away!

I meant to ask (but... verbosity overload) how closely we follow semver. If we are strict, then neither of the two changes may be made until a major version bump. It also means we are committed to mutability of PauliTerm. (You want spooky action at a distance ? I'll show you spooky action at a distance!) I suppose we could state in the doc for each new interface function that this function only works as advertised if the input data is not subsequently mutated, and we stick to it internally. But, what about composing with their previous code, or other external code ?

The percentage of users who care to know what semver is and who have looked at PauliTerm and know or care that coefficient may be changed, but not the ops is likely zero. I mean, I don't think we should rely on the presence or absence of an underscore to inform developers or users, at least in this case, where the letter and the intent do not coincide. Making clear what we assume may and may not be done to PauliTerm would cut down on the length of discussions like this.

EDIT: the proper place to start is in CONTRIBUTING.

EDIT: This would also help to sharpen thinking about the next major revision.

[appleby]

I meant to ask (but... verbosity overload) how closely we follow semver.

I'm not sure how closely we followed it in the past. I think it's up to us now!

I don't think we should rely on the presence or absence of an underscore to inform developers or users, at least in this case, where the letter and the intent do not coincide. Making clear what we assume may and may not be done to PauliTerm would cut down on the length of discussions like this.

I hear what you're saying, and there is no substitute for good documentation, but the leading underscore thing is a widely-followed Python convention, enshrined in the "official" style guide, PEP8

https://www.python.org/dev/peps/pep-0008/#public-and-internal-interfaces

And since the current docs don't explicitly say "do not mutate", someone might reasonably assume that mutating coefficient is fair game. How likely is it? 🤷‍♂

[appleby]

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."

[jlapeyre]

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

If someone ever changes

Great point. I'll add a comment.

[appleby]

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?

[braised-babbage]

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.

[jlapeyre]

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.

[appleby]

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?

[jlapeyre]

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.

[jlapeyre]

I did a quick audit of pyquil, forest-benchmarking, and other pyquil-using repos for code that sets coefficient after construction. It happens once in forest-benchmarking. But, in this case a PauliTerm is copied and coefficient is immediately changed in the copy. I opened an issue for this, since paulis.py offers an interface exactly for this purpose (rigetti/forest-benchmarking#208)

In pyquil it happens once.

This is what you get with master (and this PR branch as well)

In [1]: from pyquil.paulis import exponential_map, sZ, sY, sX, sI                                                                                  

In [2]: term = (1.0 + 0.0j) * sZ(0)                                                                                                                

In [3]: print(term)                                                                                                                                
(1+0j)*Z0

In [4]: exponential_map(term);                                                                                                           

In [5]: print(term)                                                                                                                                
1.0*Z0

[notmgsk]

I think if we can convince others that an empty program should not raise an error, then we can merge this.