Add Adj + Ctl to Assert methods that support it

[swernli]

Fixes Add Adj functor to AssertQubit #316

[bettinaheim]

@cgranade Could you take a quick look at this?

[bettinaheim]

Hm, while the controlled version could be meaningfully defined, I believe supporting it would require additional support in the C++ simulator. We need some tests for this change.

[cgranade]

What would the semantics for that assertion be? If I understand correctly, something like that the assertion fails if and only if the control qubits are in the |11…1⟩ state and the target qubits are not in the |00…0⟩ state? Regardless, we'd want to document those semantics.

[swernli]

As it stands, the controlled version below just ignores the controls, which doesn't seem to match any behavior we'd want. I didn't add that implementation, it already existed here, so I just added the Adj + Ctl characteristics for consistency (and to match our language guide recommendation of always including the characteristics). So really, I'm not changing anything here, and the question of if we want a controlled AssertAllZero and how it should behave would be a breaking change that we'd have to consider separately from this change.

[swernli]

Note that the same is true below for AssertAllZeroWithinTolerance. I can't comment on those lines directly, since they didn't change, and the diff on GitHub doesn't show them directly, so I'll include it here for better discoverability:

    operation AssertAllZero (qubits : Qubit[]) : Unit is Adj + Ctl{
        body (...) {
            for (qubit in qubits) {
                AssertQubit(Zero, qubit);
            }
        }

        adjoint self;
        controlled (ctrls, ...) {
            AssertAllZero(qubits);
        }
        controlled adjoint self;
    }

[bettinaheim]

This seems like a breaking change we might want to consider, but definitively unrelated to this PR. @cgranade I'd like to put this on your plate for API review to determine what the behavior should be and what specializations should exist.

[swernli]

@cgranade and @bettinaheim , Ping on this one. I think there is some discussion that still needs to be had about whether the existing controlled specialization for AssertAllZero is correct (likewise with AssertAllZeroWithTolerance). If we remove it, that is a breaking change that will also prevent these asserts from being used in controlled operations. The more I think about it, the more I think it actually is the right behavior and we should push that down into the intrinsic assert operations and up into the rest of the variants. Imagining a user defined callable like this:

operation customOp (qubits : Qubit[]) : is Ctl {
    AssertAllZero(qubits);
    within {
        ApplyToEachC(H, qubits);
    }
    apply {
        // some sequence of operations...
    }
}

The only intent there is to verify that the target qubits are Zero. The controls don't matter, but being able to assert inside of a controllable operation does matter.

[cgranade]

@cgranade and @bettinaheim , Ping on this one.

My apologies for missing your ping!

I think there is some discussion that still needs to be had about whether the existing controlled specialization for AssertAllZero is correct (likewise with AssertAllZeroWithTolerance). If we remove it, that is a breaking change that will also prevent these asserts from being used in controlled operations.

Agreed, and breaking changes aren't to be taken lightly.

The more I think about it, the more I think it actually is the right behavior and we should push that down into the intrinsic assert operations and up into the rest of the variants. Imagining a user defined callable like this:

operation customOp (qubits : Qubit[]) : is Ctl {
    AssertAllZero(qubits);
    within {
        ApplyToEachC(H, qubits);
    }
    apply {
        // some sequence of operations...
    }
}

The only intent there is to verify that the target qubits are Zero. The controls don't matter, but being able to assert inside of a controllable operation does matter.

I can definitely see the logic to that, and I don't even think it's wrong at all, but it's definitely not immediately intuitive to me that that's what a controlled assertion should do. In particular, if I think of an assertion as a channel of the form Λ(ρ) = |0⟩⟨0| ρ |0⟩⟨0| (that is, as a filter), then a controlled assertion would be something that applies the identity channel if the control qubits are in any state other than |11…1⟩ and that applies Λ when the control qubits are in |11…1⟩ (e.g.: 𝐶Λ(ρ) = (𝟙 ⊗ 𝟙) ρ (𝟙 ⊗ 𝟙) − (|11…1⟩⟨11…1| ⊗ |1⟩⟨1|) ρ (|11…1⟩⟨11…1| ⊗ |1⟩⟨1|)), rather than something that applies the assertion unconditioned on the state of the control register.

To communicate that distinction to users, we'd need to be really careful in how we explain what condition is being checked and in what circumstances the assertion will fail. Taking that view, your proposal would mean that assertion operations fail when the target qubit is in the wrong state, irrespective of the state of the control register, while taking the view in the above paragraph, the assertion would fail if the target is in the wrong state and the control register is in the all-ones state.

As mentioned, I think there's a real argument to be made to do something unusual in the case of assertions — it probably makes more sense to fail irrespective of control qubits — but I worry a bit about the best way to communicate that to users, I guess?

[swernli]

Finally circling back to this PR. @cgranade, regarding your concerns of communicating the behavior to users, I propagated the remarks from the doc comments on AssertMeasurement and AssertMeasurementProbability up to the other asserts, which I think should be sufficient (or should be updated everywhere). This isn't a breaking change, because I'm only adding additional functors and not taking any away, so I don't think this needs any formal API review; if anything, this brings the other asserts in line with AssertMeasurement in a way that makes the API more consistent. Would you be comfortable signing off on this so it can go into this month's release and we can finally resolve the linked issue?

[bettinaheim]

Finally circling back to this PR. @cgranade, regarding your concerns of communicating the behavior to users, I propagated the remarks from the doc comments on AssertMeasurement and AssertMeasurementProbability up to the other asserts, which I think should be sufficient (or should be updated everywhere). This isn't a breaking change, because I'm only adding additional functors and not taking any away, so I don't think this needs any formal API review; if anything, this brings the other asserts in line with AssertMeasurement in a way that makes the API more consistent. Would you be comfortable signing off on this so it can go into this month's release and we can finally resolve the linked issue?

I'll go ahead and sign off on this, with the ping to Chris to pick it up as part of the API review for all of these asserts if there is a desire to revise the current implementation. Let's close (only) on the immediate issue of having the inconsistency here.