Fix num_qubits property on AsymetricDepolarizing Channel #4652
Conversation
dabacon
changed the title
|
The reason the other PR stalled is because @viathor wanted to add a framework that makes sure this doesn't happen in the future: #4215 (review). We can merge this and create an issue for the creation of the testing framework. |
| def num_qubits(self) -> int: | ||
| """The number of qubits""" | ||
| return self._num_qubits | ||
| # The following property was deprecated in Cirq v0.14. Please use the num_qubits methos instead. |
There was a problem hiding this comment.
| # The following property was deprecated in Cirq v0.14. Please use the num_qubits methos instead. | |
| # The following property was deprecated in Cirq v0.14. Please use the num_qubits method instead. |
There was a problem hiding this comment.
removed to move comment
There was a problem hiding this comment.
LGTM
@vtomole I actually approved #4215 but @balopat correctly pointed out that we almost never accept PRs without unit tests, so I asked for changes on his behalf. I included a suggestion about a nice way to write such a unit test in a reusable way by adding a helper function.
It seems to me the PR stalled because the author is busy preparing for their finals, which is fair ;-)
| @@ -729,7 +731,7 @@ def test_default_asymmetric_depolarizing_channel(): | |||
| assert d.p_x == 0.0 | |||
| assert d.p_y == 0.0 | |||
| assert d.p_z == 0.0 | |||
| assert d.num_qubits == 1 | |||
| d.num_qubits() == 1 | |||
There was a problem hiding this comment.
I think this should still assert.
| # @property | ||
| # def num_qubits(self) -> int: | ||
| # """"The number of qubits""" | ||
| # return self._num_qubits |
There was a problem hiding this comment.
Optional: I'd remove the commented out code and just keep the comment (which you can also move to the top of the class). Folks affected by this will likely search for "num_qubits" so as long as the comment contains that string this remains discoverable.
| @@ -19,9 +19,10 @@ | |||
|
|
|||
| import numpy as np | |||
|
|
|||
| from cirq import protocols, value | |||
| from cirq import protocols, value, _compat | |||
There was a problem hiding this comment.
pylint doesn't flag this?
There was a problem hiding this comment.
it did I just was an idiot and missed it.
|
@viathor Ah thanks for clearing that up. I didn't read the following comments on that PR after yours. |
…4652) This is a breaking change! It removes the `num_qubits` property and instead one should use the `num_qubits` method.
…4652) This is a breaking change! It removes the `num_qubits` property and instead one should use the `num_qubits` method.
num_qubitswas mistakenly added as a property to AsymetricDepolarizingChannel See #4185I don't believe there is an easy way to deprecate this because one cannot have both a property and a method on the class, the last one define is called.
I propose that we just hard break this, but if someone has a better idea, I'm all ears. I kept the code commented out in case someone goes looking for this, and we can remove this after v0.15.
#4215 which also fixes this seems to have stalled.