Setup Ruff #141
Setup Ruff #141
Conversation
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #141 +/- ##
==========================================
+ Coverage 71.91% 71.96% +0.04%
==========================================
Files 30 30
Lines 5359 5350 -9
==========================================
- Hits 3854 3850 -4
+ Misses 1505 1500 -5 ☔ View full report in Codecov by Sentry. |
| """quantum channel class in the Kraus representation. | ||
| Defined by Kraus operators :math:`K_i` with scalar prefactors :code:`coef`) :math:`c_i`, | ||
| where the channel act on density matrix as :math:`\\rho' = \sum_i K_i^\dagger \\rho K_i`. | ||
| The data should satisfy :math:`\sum K_i^\dagger K_i = I` | ||
| where the channel act on density matrix as :math:`\\rho' = \\sum_i K_i^\\dagger \\rho K_i`. | ||
| The data should satisfy :math:`\\sum K_i^\\dagger K_i = I` |
There was a problem hiding this comment.
Can we use raw-strings (as in two_qubit_depolarising_tensor_channel)?
| """two-qubit depolarising channel. | ||
|
|
||
| .. math:: | ||
| \mathcal{E} (\\rho) = (1-p) \\rho + \\frac{p}{15} \sum_{P_i \in \{id, X, Y ,Z\}^{\otimes 2}/(id \otimes id)}P_i \\rho P_i | ||
| \\mathcal{E} (\\rho) = (1-p) \\rho + \\frac{p}{15} \\sum_{P_i \\in \\{id, X, Y ,Z\\}^{\\otimes 2}/(id \\otimes id)}P_i \\rho P_i |
There was a problem hiding this comment.
Can we use raw-strings (as in two_qubit_depolarising_tensor_channel)?
| except ImportError: | ||
| raise ImportError( # noqa: B904 (Just rewriting error message) |
There was a problem hiding this comment.
I would prefer a proper raise ... from even if it is not that useful rather than silencing a warning (it can even be raise ... from None if we really don't want exception chaining).
| @@ -501,11 +501,10 @@ def equivalent_fill_node(self, node: int) -> int: | |||
| self.equivalent_graph_E1(i) # this gives loop to node. | |||
| self.equivalent_graph_E1(node) | |||
| return 0 | |||
| elif len(list(self.neighbors(node))) == 0: | |||
There was a problem hiding this comment.
It may wait another PR but since we are going to change this line anyway, we have the opportunity to change this test into: elif all(False for _ in self.neighbors(node)):.
There was a problem hiding this comment.
self.neighbors(node) returns an iterator: converting it into a list for computing its length costs O(n) just to check whether the iterator is empty, whereas all returns False as soon as the iterator returns one element, therefore it is O(1).
| k = list(f[k])[0] | ||
| pos[k][1] = i | ||
| kk = next(iter(f[k])) | ||
| pos[kk][1] = i |
There was a problem hiding this comment.
The purpose of this while loop is to make k follow the path given by f: it is essential for k to be updated during the loop, or else the loop never terminates (because k will never change). If the purpose of this change is to prevent updating a for-loop variable, a solution could be:
node = k
while node in f.keys():
node = next(iter(f[node]))
pos[node][1] = iThere was a problem hiding this comment.
(Note: we are missing test_visualization.py to catch that!)
There was a problem hiding this comment.
the purpose of this change is to prevent updating a for-loop variable
This is exactly what I meant.
Anyway we need to cover these lines...
There was a problem hiding this comment.
I propose a fix and a test here: thierry-martinez@d761465
| @@ -176,5 +176,5 @@ def test_backward_substitute(self, test_case: LinalgTestCase) -> None: | |||
| mat_eliminated, rhs_eliminated, _, _ = mat.forward_eliminate(rhs_input) | |||
| x, kernel = mat_eliminated.backward_substitute(rhs_eliminated) | |||
| if x is not None: | |||
| assert np.all(x == x) | |||
| assert np.all(x == x) # noqa: PLR0124 (necessary) | |||
There was a problem hiding this comment.
Could we replace that with: assert not np.any(np.isnan(x)), which looks clearer?
There was a problem hiding this comment.
I'm afraid not.
As far as I tested, it leads to TypeError because x can sometimes be object-valued and thus we cannot cast the elements into float.
There was a problem hiding this comment.
Oh, indeed, this test is suspicious: there are actually some NaNs in the values generated by the tests (for instance, for the test case not full-rank matrix, x == [x0 nan]), but nan are not float but instances of sympy.core.numbers.NaN, for which the equality is reflexive (i.e., sympy.core.numbers.NaN() == sympy.core.numbers.NaN(); see sympy/sympy#7797)!
A fix could be assert np.all(x != sympy.core.numbers.NaN()), but then the test fails because of NaN values.
This looks out of the scope of this PR and fixed in another PR. @shinich1 , @masa10-f, what do you think about this?
@thierry-martinez @shinich1 is not planning to merge this PR immediately. |
Description of the change:
Setup Ruff linter CI.
Related issue:
Resolves #140