Single qbtgates changes

[MartinSandeCosta]

Description

• Added controlled version of the single-qubit gates
• Added controlled single-qubit gates to Qubit.Qip.Circuit.resolve_gates()
• Added controlled single-qubit gates to Qubit.Qip.Circuit.propagator()
• Added controlled single-qubit gates to Qubit.Qip.Circuit.latex_code()
• Added controlled single-qubit gates to Qubit.Qip.Circuit.add_1q_gate()
• Added tests for controlled single-qubit gates in Qubit.Qip.Operations.Gates and Qubit.Qip.Circuit

Related issues or PRs
fixes #1225

Changelog
The controlled version of the single-qubit gates and tests

[MartinSandeCosta]

Notebook added

[MartinSandeCosta]

These tests seem a little too scant in my opinion, I am not fully satisfied with the final result. They do not provide full coverage of these new functionalities.

I am thinking of refactoring the whole thing and start all over again.

[ajgpitch]

@MartinSandeCosta well, it is most important that you are happy with PR. Let us know when you have something you want us to review. Thanks

[ajgpitch]

closes #1225

A little more detail in the opening comment is preferred. It helps whoever has to update the changelog.

[BoxiLi]

@MartinSandeCosta, free feel to complete the tests in the way you want. However, maybe it is good to discuss a bit with @jakelishman? Since there is a refactoring of qutip/tests/test_gates.py in #1181. A lot of change in this test might make it hard to resolve two PRs later.

Besides, it is better to leave a more detailed description in the PR. As the template indicated:

**Description**
Describe here the proposed change.

**Related issues or PRs**
Please mention the related issues or PRs here. If the PR fixes an issue, use the keyword fix/fixes/fixed followed by the issue id, e.g. fix #1184

**Changelog**
Give a short description of the PR in a few words. This will be shown in the QuTiP change log after the PR gets merged.
For example: 
Fixed error checking for null matrix in essolve.
Added option for specifying resolution in Bloch.save function.

[jakelishman]

Hmmm. You're right that there will be some nasty merge conflicts with #1181, but I don't see anyway around those, to be honest without that PR being merged before this one (which doesn't look very likely). Barring #1181 getting merged in the next day or two, I'd just go ahead with these commits as written.

If the new-style tests were merged, test_gates.py would have a much tinier diff. The single-qubit gates would just go between lines 222 and 223 in

qutip/qutip/tests/test_gates.py

Lines 222 to 239 in 77c21f8

	@pytest.mark.parametrize(["gate", "n_angles"], [
	pytest.param(gates.rx, 1, id="Rx"),
	pytest.param(gates.ry, 1, id="Ry"),
	pytest.param(gates.rz, 1, id="Rz"),
	pytest.param(gates.phasegate, 1, id="phase"),
	pytest.param(gates.qrot, 2, id="Rabi rotation"),
	])
	def test_single_qubit_rotation(self, gate, n_angles):
	base = qutip.rand_ket(2)
	angles = np.random.rand(n_angles) * 2*np.pi
	applied = gate(*angles) * base
	random = [qutip.rand_ket(2) for _ in [None]*(self.n_qubits - 1)]
	for target in range(self.n_qubits):
	start = qutip.tensor(random[:target] + [base] + random[target:])
	test = gate(*angles, self.n_qubits, target) * start
	expected = qutip.tensor(random[:target] + [applied]
	+ random[target:])
	assert _infidelity(test, expected) < 1e-12

as

    pytest.param(gates.x_gate, 0, "X"),
    pytest.param(gates.y_gate, 0, "Y"),
    ...

(and we'd probably change the function name to test_one_qubit). The controlled qubits would similarly go into the next test's parametrisation list:

qutip/qutip/tests/test_gates.py

Lines 241 to 259 in 77c21f8

	@pytest.mark.parametrize(['gate', 'n_controls'], [
	pytest.param(gates.cnot, 1, id="cnot"),
	pytest.param(gates.swap, 0, id="swap"),
	pytest.param(gates.iswap, 0, id="iswap"),
	pytest.param(gates.sqrtswap, 0, id="sqrt(swap)"),
	pytest.param(functools.partial(gates.molmer_sorensen, 0.5*np.pi), 0,
	id="Molmer-Sorensen")
	])
	def test_two_qubit(self, gate, n_controls):
	targets = [qutip.rand_ket(2) for _ in [None]*2]
	others = [qutip.rand_ket(2) for _ in [None]*self.n_qubits]
	reference = gate() * qutip.tensor(*targets)
	for q1, q2 in itertools.permutations(range(self.n_qubits), 2):
	qubits = others.copy()
	qubits[q1], qubits[q2] = targets
	args = [[q1, q2]] if n_controls == 0 else [q1, q2]
	test = gate(self.n_qubits, *args) * qutip.tensor(*qubits)
	expected = _tensor_with_entanglement(qubits, reference, [q1, q2])
	assert _infidelity(test, expected) < 1e-12

Overall, it's clear that you can't write sensible new-style tests without #1181 having been merged, so I wouldn't worry about it too much. I did drop in a couple of comments in-line (minor nitpicking), but given that this file may well get pretty much overwritten - keeping the actual tests, of course! - in short order, I wouldn't consider them changes that need to be made, unless Boxi asks.

Also, as you noted, test_gates.py doesn't really do much testing of the actual form of the gates yet. I haven't actually been adding new tests as I go, because I wanted to keep #1181 roughly focussed on just converting what we've got so far.

[jakelishman]

These are just nitpicks, and a couple of comments on how we might have done things in the new pytest way. In general I think you did a very good job of writing code that's presumably not your natural style, but is well in keeping with everything else in the files!

[jakelishman]

Again, I know you're just following what's going on in the rest of the file (and that's typically a very good thing!). Now that we've moved to using pytest, np.testing.assert_ is unnecessary - you can just use the keyword assert. It used to be used because numpy abstracted their own tests away from the framework that they used to run them (sort of), and then this provided consistency. Nowadays, pytest will do introspection on any variables that appear in a bare assert statement and print out their values, so we actually get much better error reporting by just doing assert.

[jakelishman]

If we were to keep using the np.testing functions (which we don't need to because pytest makes it all easier), the better one here would be np.testing.assertEqual (even for the None test).

We always should think about what information we want to get out of a failed test. In this case, we'd be best served by knowing what the incorrect value was, so we should pass the two parts of the comparison separately. That way, np.testing.assertEqual will print them out on a test failure, whereas this way np.testing.assert_ just gets fed the value False and we wouldn't know what it was (was it 3, 0, "", "None"?) and we'd have to do more work offline to find out.

[jakelishman]

In new pytest-style, the for loop would be done with parametrised tests and the raising test uses the pytest.raises context manager, so you get easier-to-read code (and it's selectable if you only want to test a certain gate) like

@pytest.mark.parametrize('gate', ['X', 'Y', 'Z', 'S' ,'T'])
def test_exceptions(self, gate):
    qc = QubitCircuit(2)
    with pytest.raises(ValueError):
        qc.add_gate(gate, targets=[1], controls=[0])

Again, thinking of the test log, this way is nice because the test log will tell you exactly which gate failed - pytest adds the parametrised arguments into the test name.

[MartinSandeCosta]

Thank you for taking the time to look at these changes, @jakelishman! It must have been a true nightmare looking at my code haha 😄

Now on a more formal note. Regarding the pytest side of things, should I reformat the tests (related to my single_qubit implementation) in test_qubitcircuit.py and test_gates to follow the changes in #1181? It would definitely make it easier to merge them later on.

[jakelishman]

Nah, don't disparage yourself - your code is good and you've done well to copy the style of the rest of the files. Typically consistency is better than having "the one true style"; code's read more than it's written, and having a module which is clearly written by four different people makes the logic very very difficult to follow, which in turns makes it much harder to spot bugs.

If it were up to me (which is isn't, really), I would say that your changes are good as they are. I didn't mean for the comments to be changes you had to make. You can't reduce the number of merge conflicts that are going to happen with #1181 anyway, because that PR is a massive refactor of large swathes of the testing suite. You've done a good job to keep your work consistent with the rest of the file, so it will be very easy to merge in your logical changes. Merge conflicts are pretty much just par for the course; just the fact that we're discussing this here shows that logically, there is a conflict between two pieces of work being done at the same time.

I'd suggest merging this as-is. We'll handle the problems caused by the refactor elsewhere.

[BoxiLi]

Hi @MartinSandeCosta, you've done very well. As @jakelishman said, keeping thing consistent with the rest is definitely good. It's just because there have been several waves of development of QuTiP and the coding style is not always consistent with each other, as you may already see. And it happens that @jakelishman is modernizing the tests right at this moment.

There is an API change that we'd better not do in this PR, otherwise, everything is good. I'll merge it then and we can deal with the conflicts in the tests later. Good job!

[BoxiLi]

@MartinSandeCosta Congrats!

By the way, the changelog in the PR description will later be used in the documentation (http://qutip.org/docs/latest/changelog.html). It's better to be a summary of the PR in one-line :)