Move controlled gate tests from Aqua to Terra#3714
Conversation
register names are more expressive now, also removed names of register, since this is not needed. list comprehension was removed where the whole register could be passed instead.
Cryoris
requested review from
1ucian0,
ajavadia,
ewinston,
kdk and
mtreinish
as code owners
1ucian0
left a comment
1ucian0
left a comment
There was a problem hiding this comment.
The test are very hard to understand. I would prefer simpler and cleaner tests. Is that doable?
For a given set of control qubits, the tests build the matrix representation for the operation via the unitary simulator and compare it against the theoretical matrix. That's also what other tests (such as |
|
Does this test something different than what's tested by |
We can, with the functionality from #3739 😄 |
| output_ctrl != input_ctrl) | ||
| self.assertTrue( | ||
| ((output_ctrl == input_ctrl) and (output_target != cond_output)) or | ||
| output_ctrl != input_ctrl) |
There was a problem hiding this comment.
Having different assert under a condition tells me that these should be two different tests. What do you think? Would it be possible to split these situations ?
There was a problem hiding this comment.
This test was already in the master branch (my editor probably changed the format without me noticing), I think these tests are from @ewinston. Probably the entire condition in the assert must be fulfilled, or can they be split?
There was a problem hiding this comment.
I put this into a single test because it is a validation of a truth table; the clause just separates truth and false conditions.
1ucian0
left a comment
1ucian0
left a comment
There was a problem hiding this comment.
It looks good! I had some small comments here and there.
* port qc.mct tests * port qc.mcr(x,y,z) tests * port qc.mcu1 test * update register names, remove unnecessary list comprehension register names are more expressive now, also removed names of register, since this is not needed. list comprehension was removed where the whole register could be passed instead. * docstrings have complete sentences * fix lint, remove "parameterized" dependency * use @combine instead of @unpack@data(itertools)) * fix import order * self.assertTrue(np.allclose to assert_allclose * atol as np.allclose * unfold test_multi_control_toffoli_matrix_dirty_ancillas modes * no need for combine in test_multi_control_toffoli_matrix_clean_ancillas * unify style * import gates via allGates * unfold test_all_inverses * unfolding test_multi_control_toffoli_matrix * simplification * use _compute_control_matrix where possible * fix lint * use subtest more, add note that tests from Aqua Co-authored-by: Luciano Bello <luciano.bello@ibm.com>
3 participants
Summary
Some multi-controlled gates have been moved from Aqua to Terra without their tests. This PR adds all existing tests for these gates that exist in Aqua. This moves #3639 forward.
Details and comments
Tests for the following files exist currently in Aqua but not in Terra:
qc.mct: multi-control Toffoli gateqc.mcu1: multi-control U1 gateqc.mcr(x,y,z): multi-control rotation gatesThis PR integrates the tests from Aqua's
test/aqua/test_<gatename>.pyin Terra'stest/python/circuit/test_controlled_gate.py. Note, that also the relative phase Toffoli gates were moved to Terra without tests, but also Aqua doesn't have any and therefore none are included in this PR. This should be done in a future PR.Also the docstrings for all tests were updated to be complete sentences and made a bit more descriptive for some tests than before.