Kak

[kodack64]

実装ありがとうございます。KAKの実装は大変だったかと思いますがお疲れ様です。 Visual studioのデバッグビルドなどでも問題なく通りました。ソースコードの記述について何点かコメントがあります。 （qulacs-osakaを大阪大学で独立して作るならコードの細かい部分よりスピード重視で良いと思いますが、qulacsに統合していくとのことで、qulacsは英語でissueが上がることもあるので、英単語なども一応コメントを入れています。）

KAK.hppについて

1. ソースコード中のコメントは出来れば英語の方が望ましいです。 両方が実数の場合の、同時特異値分解します -> Bidiagonalize real matrix pairs 入力は4*4のゲート -> The input is assumed to be a gate with 4x4 matrix

上のKAK分解は実装を省略しているため、そのままだと縮退?などにより、正しい値が出ない可能性がありました。それを解消するため、分解したい行列に対して、各ビットにランダムユニタリを掛けてからKAK分解し、　その結果にさっきかけたランダムユニタリの逆行列を掛けることで対処しています.

->

The above KAK decomposition partially omit the implementation, and the function somtimes outputs invalid values. To address this problem, we apply a random matrix to the input unitary, perform the KAK decomposition, and retrieve the correct answer by applying its inverse. We expect that this is due to the degeneration of matrix eigenvalues.

また、明示的に何らかの処理をしている実装を省略しているとしたら、どのように実装を省略しているのかを記載しておきましょう。また、このコードはcirqの実装をベースに作ら得ているので、ファイルの冒頭でそのことに振れておいた方がいいかなと思います。

2. L116-L121のコメントは消しましょう

3. 何点か日本語由来と思われる変数名や文字リテラルがありますが、海外の方がソースコードを見たときに混乱するため英語にしておきましょう。

rank fusoku Internal error -> Rank is too small

mat_motoという変数名がありますが、motoが「元の」という内容を意味しているなら、mat_orginalなどにしましょう。（moto...という英単語の接頭句を指しているならそれでもいいです）

target_bits is 2 please -> Target qubits must be 2 sort please -> KAK decomposition assumes that the provided qubit indices are sorted, but unsorted arguments are given. dont target_gate includes control qubit -> do not include control qubits in target_gate CSD qubit size>=2 please -> CSD qubit size must be no less than 2

answerという変数名をansとする程度は問題ないのですが、aaa_gate1のように変数名に意味のない単語をつけるのは出来ればやめましょう。他の人が後からコードを読むときのことを考え、一時的な変数であるにせよそのことが分かるようにした方が良いです。

4. CSDのコードが含まれていますが、CSDとKAKは異なる行列分解で、CSDがKAK.hppに実装されているのは違和感があるので、ファイル名をmatrix_decomposition.hppなどにするか、CSDの実装をCSD.hppというファイルに分けましょう。

その他のファイル

include_all.hppを実装していますが、まず、includeするファイルが自身のライブラリのローカルファイルであることが分かっている場合、<>ではなく""でくくる方が望ましいと思います。また、全てのファイルをincludeするのはコンパイル時間が長くなるため、基本的にあまり推奨されません。手間はかかるかもしれませんが、できれば必要なファイルで必要なファイルのみをincludeする方が望ましいです。現状このファイルは使われていないように見えますが、一時的に追加している場合はcommitから除くのが良いと思います。

test_KAK.cppの#include <fstream>は不要かなと思いました。

[WATLE]

対応しました

[codecov-commenter]

Codecov Report

Merging #399 (ac1d6d6) into dev (24e7aa0) will increase coverage by 1.43%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #399      +/-   ##
==========================================
+ Coverage   70.95%   72.38%   +1.43%     
==========================================
  Files          84       84              
  Lines        5970     6106     +136     
==========================================
+ Hits         4236     4420     +184     
+ Misses       1734     1686      -48     

Impacted Files	Coverage Δ
src/cppsim/exception.hpp	14.28% <ø> (+0.64%)	:arrow_up:
src/cppsim/gate_factory.cpp	58.76% <ø> (+0.68%)	:arrow_up:
src/cppsim/gate_noisy_evolution.hpp	83.62% <100.00%> (+4.04%)	:arrow_up:
src/cppsim/qubit_info.hpp	76.47% <0.00%> (+3.13%)	:arrow_up:
src/cppsim/gate_merge.cpp	86.66% <0.00%> (+4.24%)	:arrow_up:
src/csim/update_ops_pauli_single.cpp	91.66% <0.00%> (+6.25%)	:arrow_up:
src/cppsim/pauli_operator.cpp	78.82% <0.00%> (+7.54%)	:arrow_up:
src/csim/stat_ops_expectation_value.cpp	87.34% <0.00%> (+8.86%)	:arrow_up:
... and 4 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[WATLE]

謎のエラーでCI落ちました、助けてください

[kodack64]

返信が遅くなりました。ありがとうございます。(kotamanegiさんもありがとうございます) ２点だけ下記の修正を入れていただいたらマージしてokです。自明な変更なのでapproveは入れておきます。

1. L190付近のコメント

    Since the KAK decomposition above omits the implementation, there is a
    possibility that the correct value will not come out due to degeneration
    etc. if it is as it is. In order to solve this problem, we deal with the
    matrix we want to decompose by multiplying each bit by random unitary, KAK
    decomposition, and multiplying the result by the inverse matrix of random
    unitary that we just applied.

は、以下の文章にしましょう。

Since the above KAK decomposition omits a part of the implementation from the reference (Cirq's implementation), 
wrong values may be obtained with this function due to parameter degeneration.
To deal with this problem, we multiply each qubit by 2x2 random unitary matrices, perform KAK decomposition, 
and multiply the result by the inverse matrix of the random unitary matrices.

2. テストのファイル名はtest_KAK.cppからtest_matrix_decomposition.cppにしましょう。

内部コメントを日本語にするかどうかは議論の余地があると思います。 こちらについては別途slackで議論しましょう。

[kodack64]

ちなみにCIについて、以前からファイルのfetchに失敗するなどでランダムに失敗したという経験が私もあります。 （おそらく、workflowの全体に関する設定が古くなったりしたことによる問題ではないかと思います。） この場合、CIの画面からworkflowのリスタートができてそれで何とかなることがあるので、それを試してみてください。 権限の関係でworkflowのリスタートが出来ない場合はslackで聞いてください。