This repository has been archived by the owner on Dec 7, 2021. It is now read-only.
Make ListOp.num_qubits check all ops for same num_qubits #1189
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cryoris
reviewed
Aug 13, 2020
Comment on lines
454
to
464
| op = ListOp([X ^ Y, Y ^ Z]) | ||
| self.assertEqual(op.num_qubits, 2) | ||
| op = ListOp([X ^ Y, Y]) | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| op.num_qubits | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| X @ op |
There was a problem hiding this comment.
Could you wrap these in sub-tests, one for the ListOp with equal number of qubits and one for the other one?
There was a problem hiding this comment.
(somehow missed this comment earlier!) Yes.
It might be a good idea to test that op @ X (as opposed to X @ op) does not raise an error.
Cryoris
reviewed
Aug 13, 2020
| @@ -129,7 +129,10 @@ def primitive_strings(self) -> Set[str]: | |||
|
|
|||
| @property | |||
| def num_qubits(self) -> int: | |||
| return self.oplist[0].num_qubits | |||
| num_qubits0 = self.oplist[0].num_qubits | |||
| if not all(num_qubits0 == op.num_qubits for op in self.oplist): | |||
There was a problem hiding this comment.
Suggested change
| if not all(num_qubits0 == op.num_qubits for op in self.oplist): | |
| if not all(num_qubits0 == op.num_qubits for op in self.oplist[1:]): |
There was a problem hiding this comment.
self.oplist[1:] is likely less efficient than self.oplist because the former allocates and makes a copy of the list, whereas the latter adds one dereference and integer comparison... On the other hand, efficiency is not a design criterion for aqua, and using the slice signals algorithmic intent.
There was a problem hiding this comment.
The fact that it ends up checking itself is fine - what we care about is that they are all the same, whichever one we pick as some reference.
jlapeyre
force-pushed
the
listop-check-num-qubits
branch
2 times, most recently
from
d84b0e5 to
e4354b9
Compare
Previously, the number of qubits in the first operator in the ListOp was returned. With this change, an additional check is made that all other operators also have the same number of qubits. Closes qiskit-community#1086
jlapeyre
force-pushed
the
listop-check-num-qubits
branch
from
e4354b9 to
fcaf075
Compare
|
Is this ready to go? Still marked [WIP] |
|
|
Can we move forwards on this. It seems fine to me. |
|
Yes. I let this slip. |
jlapeyre
force-pushed
the
listop-check-num-qubits
branch
from
d1f2125 to
a3e829a
Compare
jlapeyre
changed the title
woodsp-ibm
approved these changes
Aug 20, 2020
Cryoris
pushed a commit
to Cryoris/qiskit-aqua
that referenced
this pull request
Sep 3, 2020
…unity#1189) * Make ListOp.num_qubits check all ops for same num_qubits Previously, the number of qubits in the first operator in the ListOp was returned. With this change, an additional check is made that all other operators also have the same number of qubits. Closes qiskit-community#1086 * Add release note for listop-check-num-qubits Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
pbark
pushed a commit
to pbark/qiskit-aqua
that referenced
this pull request
Sep 16, 2020
…unity#1189) * Make ListOp.num_qubits check all ops for same num_qubits Previously, the number of qubits in the first operator in the ListOp was returned. With this change, an additional check is made that all other operators also have the same number of qubits. Closes qiskit-community#1086 * Add release note for listop-check-num-qubits Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
mtreinish
pushed a commit
to mtreinish/qiskit-core
that referenced
this pull request
Nov 20, 2020
…unity/qiskit-aqua#1189) * Make ListOp.num_qubits check all ops for same num_qubits Previously, the number of qubits in the first operator in the ListOp was returned. With this change, an additional check is made that all other operators also have the same number of qubits. Closes qiskit-community/qiskit-aqua#1086 * Add release note for listop-check-num-qubits Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
manoelmarques
added a commit
to manoelmarques/qiskit-terra
that referenced
this pull request
Dec 2, 2020
…unity/qiskit-aqua#1189) * Make ListOp.num_qubits check all ops for same num_qubits Previously, the number of qubits in the first operator in the ListOp was returned. With this change, an additional check is made that all other operators also have the same number of qubits. Closes qiskit-community/qiskit-aqua#1086 * Add release note for listop-check-num-qubits Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
manoelmarques
added a commit
to manoelmarques/qiskit-terra
that referenced
this pull request
Dec 7, 2020
…unity/qiskit-aqua#1189) * Make ListOp.num_qubits check all ops for same num_qubits Previously, the number of qubits in the first operator in the ListOp was returned. With this change, an additional check is made that all other operators also have the same number of qubits. Closes qiskit-community/qiskit-aqua#1086 * Add release note for listop-check-num-qubits Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the number of qubits in the first operator in the ListOp
was returned. With this change, an additional check is made that all
other operators also have the same number of qubits.
To be clear: This check that the number of qubits is the same for each operator does not occur on construction of the
ListOp. Rather, it occurs whennum_qubitsis accessed. This will cause some nonsensical aggregate constructions involvingListOpto raise an error, rather than succeed. For exampleX @ ListOp([X, Y^Z]), will now raise aValueError, rather than succeeding as before. However,ListOp([X, Y^Z]) @ Xwill still succeed. This should perhaps be changed, but is beyond the scope of this PR, which is the behavior ofListoP.num_qubits.There is an alternative PR #1118 open for the same issue. However the present PR is probably the preferred solution.
Closes #1086
X I have added the tests to cover my changes.
✅ I have updated the documentation accordingly.
✅ I have read the CONTRIBUTING document.
There is an alternative PR open for the same issue: #1118 . However the present PR is probably the preferred solution.