takagi fix #394

RyosukeNORO · 2024-07-05T16:01:40Z

Context:
There were issues with Takagi decomposition, specifically sqrtm, with some matrix. For the previous discussion, please see (#393)

Description of the Change:
added the calculation method when the matrix to be takagi decomposed is diagonal.
added the test code of the matrix for which the decomposition did not work well to test_decompositions.py.

Benefits:
Takagi decomposition works well.

Possible Drawbacks:

Related GitHub Issues:
(#393)

… matrix to test_decompositions.py.

elib20 · 2024-07-05T16:08:47Z

@timmysilv, what do you suggest we do about codefactor here? We have a function with a lot of edge cases where the computation simplifies, so we'd like to leverage that, but now it is complaining about too many return statements.

nquesada · 2024-07-05T16:11:11Z

thewalrus/decompositions.py

@@ -202,6 +202,15 @@ def takagi(A, svd_order=True):
        vals, U = takagi(Amr, svd_order=svd_order)
        return vals, U * np.exp(1j * phi / 2)

+    # If the matrix is diagonal, Takagi decomposition is easy
+    if np.allclose(A, np.diag(np.diag(A))):


You need an rtol inside the np.allclose

nquesada · 2024-07-05T16:12:03Z

thewalrus/decompositions.py

+        d = np.diag(A)
+        U = np.diag(np.exp(1j * 0.5 * np.angle(d)))
+        l = np.abs(d)
+        if svd_order is False:


You need to correct this. First you need to sort l and rearrange U accordingly. Then you can decide if you flip the order. As is, this will not return the takagi singular values sorted correctly.

nquesada · 2024-07-05T16:13:33Z

thewalrus/tests/test_decompositions.py

+def test_takagi_sepcific_matrix():
+    """Test the takagi decomposition works well for a specific matrix that was not deecomposed accuratelyin a previous version.
+    See more info in PR #393 (https://github.com/XanaduAI/thewalrus/pull/393)"""
+    A = np.load('test_matrix_for_takagi.npy')


You should write the matrix explicitly here instead of importing it from a npy file. It is, after all, just a 3x3 matrix so it should not occupy a lof of space. If you want to minimize the space, you could write separately the real and imaginary part and the construct the matrix by summing them.

nquesada

Nice. Needs a few tweaks before approval.

thewalrus/tests/test_decompositions.py

nquesada · 2024-07-05T16:16:03Z

@elib20 : for the complaint about too many returns you add #pylint ignore to tell it to shut up ;)

Co-authored-by: Nicolas Quesada <[email protected]>

and added the diagonal matrix explicitly to test_decompositions.py.

into takagi-sqrtm-fix

thewalrus/tests/test_decompositions.py

nquesada

Nice. Basically there modulo small typos.

Co-authored-by: Nicolas Quesada <[email protected]>

nquesada · 2024-07-05T18:13:37Z

thewalrus/tests/test_decompositions.py

+from thewalrus.decompositions import (
+    williamson,
+    blochmessiah,
+    takagi,


did you do black -l 100 file.py it seems to me like this changes should not happen.

I did black file.py. Should I do black -l 100 file.py instead?

Yup, that is the standard use in here.

timmysilv

I've added an in-line suggestion for how to fix this. I also think mathematical functions with many branches are reasonable to have (provided they remain legible which this is imo), and I'm pretty sure you can avoid pylint warnings from Codefactor like any other time (adding pylint: disable=too-many-return-statements to the function definition on line 155)

EDIT: I see the pylint disable comment above so I guess it should work 👍

timmysilv · 2024-07-05T18:18:13Z

thewalrus/decompositions.py

+        if svd_order:
+            return l[::-1], U[:, ::-1]
+        return l, U


you can do the same thing on lines 218-220 as well if it helps, but I think this should suffice.

Suggested change

if svd_order:

return l[::-1], U[:, ::-1]

return l, U

return (l[::-1], U[:, ::-1]) if svd_order else (l, U)

fixed the ordering of the matrix in takagi with diagonal matrix

codecov · 2024-07-05T20:05:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f935053) to head (b5fbd06).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master      #394   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines         1996      1912   -84     
=========================================
- Hits          1996      1912   -84

Files with missing lines	Coverage Δ
thewalrus/decompositions.py	`100.00% <100.00%> (ø)`

... and 25 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f935053...b5fbd06. Read the comment docs.

nquesada · 2024-07-05T20:14:05Z

Almost there @RyosukeNORO In the test that you added you need to add an extra line to trigger the lines where the order of the takagi values and unitaries are flipped. With that you should get full coverage.

added test to trigger both svd_order=True and False in takagi with diagonal matrix

nquesada · 2024-07-06T20:31:09Z

thewalrus/decompositions.py

@@ -202,6 +203,19 @@ def takagi(A, svd_order=True):
        vals, U = takagi(Amr, svd_order=svd_order)
        return vals, U * np.exp(1j * phi / 2)

+    # If the matrix is diagonal, Takagi decomposition is easy
+    if np.allclose(A, np.diag(np.diag(A)), rtol=1e-16):


I'd suggest you move this rtol as an optional parameter in the signature of the function.

I have moved rtol as an optional parameter of takagi function.

nquesada

Great job @RyosukeNORO !

added the calculation method of diagonal matrix to takagi, and test…

c41b49f

… matrix to test_decompositions.py.

RyosukeNORO changed the title ~~added the calculation method of diagonal matrix to takagi, and test…~~ takagi fix Jul 5, 2024

nquesada reviewed Jul 5, 2024

View reviewed changes

nquesada requested changes Jul 5, 2024

View reviewed changes

nquesada reviewed Jul 5, 2024

View reviewed changes

thewalrus/tests/test_decompositions.py Outdated Show resolved Hide resolved

added the related pull request number into CHANGELOG.md.

3ff6505

RyosukeNORO and others added 3 commits July 5, 2024 13:43

Update thewalrus/tests/test_decompositions.py

6e9b2b5

Co-authored-by: Nicolas Quesada <[email protected]>

added rtol to np.allclose, corrected the sort of l and U,

137820e

and added the diagonal matrix explicitly to test_decompositions.py.

Merge branch 'takagi-sqrtm-fix' of https://github.com/XanaduAI/thewalrus

1ae82f1

into takagi-sqrtm-fix

nquesada reviewed Jul 5, 2024

View reviewed changes

thewalrus/tests/test_decompositions.py Outdated Show resolved Hide resolved

nquesada reviewed Jul 5, 2024

View reviewed changes

RyosukeNORO and others added 2 commits July 5, 2024 13:55

reformat test_decompositions.py using black

05ed19d

Update thewalrus/tests/test_decompositions.py

7f1a795

Co-authored-by: Nicolas Quesada <[email protected]>

nquesada reviewed Jul 5, 2024

View reviewed changes

timmysilv reviewed Jul 5, 2024

View reviewed changes

added pylint: disable=too-many-return-statements.

cf82481

fixed the ordering of the matrix in takagi with diagonal matrix

RyosukeNORO added 2 commits July 5, 2024 18:02

fixed the ordering of the matrix in takagi with diagonal matrix

74c05b8

added test to trigger both svd_order=True and False in takagi with diagonal matrix

reformated test_decompositions.py using black -l 100

c81cfbb

nquesada reviewed Jul 6, 2024

View reviewed changes

moved the rtol as an optional parameter of takagi

6fb38fe

nquesada approved these changes Jul 8, 2024

View reviewed changes

updeted the description of rtol in takagi

b5fbd06

RyosukeNORO merged commit 0e51518 into master Jul 8, 2024
5 checks passed

RyosukeNORO deleted the takagi-sqrtm-fix branch July 8, 2024 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

takagi fix #394

takagi fix #394

RyosukeNORO commented Jul 5, 2024 •

edited

Loading

elib20 commented Jul 5, 2024

nquesada Jul 5, 2024

nquesada Jul 5, 2024

nquesada Jul 5, 2024

nquesada left a comment

nquesada commented Jul 5, 2024

nquesada left a comment

nquesada Jul 5, 2024

RyosukeNORO Jul 5, 2024

nquesada Jul 5, 2024

timmysilv left a comment •

edited

Loading

timmysilv Jul 5, 2024

codecov bot commented Jul 5, 2024 •

edited

Loading

nquesada commented Jul 5, 2024

nquesada Jul 6, 2024

RyosukeNORO Jul 8, 2024

nquesada left a comment

takagi fix #394

takagi fix #394

Conversation

RyosukeNORO commented Jul 5, 2024 • edited Loading

elib20 commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nquesada left a comment

Choose a reason for hiding this comment

nquesada commented Jul 5, 2024

nquesada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmysilv left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

nquesada commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nquesada left a comment

Choose a reason for hiding this comment

RyosukeNORO commented Jul 5, 2024 •

edited

Loading

timmysilv left a comment •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading