ParametricPauliRotationゲートをbackprop対象にする

forest1040 commented 2 years ago

PauliRotationGateですので、pauli_id_listが必要なため、ClsParametricPauliRotationGateクラスにget_pauli()を追加しました。 get_pauli()は、ClsParametricPauliRotationGateクラスではなく、もっとベースクラスに追加した方がよいかも知れません。。そもそもpauli_id_listを取得するのに、ClsParametricPauliRotationGateクラスを使うことが適切かどうかも見て頂きたいです。まだqulacs-osakaのコードをざっとしか見れておりませんので検討違いかも知れません。確認をお願い致します。

forest1040 commented 2 years ago

私の環境では、私の修正前のコードでも以下の単体テストでエラーになるようです。。 Docker上で開発しております。 https://github.com/forest1040/qulacs-osaka/blob/dev-local/docker/gpu/Dockerfile

CONTRIBUTING.mdの以下の手順を実行しました。

$ ./script/build_gcc.sh
$ python setup.py install
$ cd build
$ make test
$ make pythontest

make testは100%OKですが、make pythontestでエラーになります。

$ make pythontest
[  8%] Built target eigen
[ 17%] Built target Cereal
[ 56%] Built target csim_static
[ 73%] Built target cppsim_static
[ 80%] Built target vqcsim_static
[ 82%] Built target qulacs_core
[ 97%] Built target cppsim_exp_static
[100%] Built target qulacs_osaka_core
...........* Warning : CPTP-map was not trace preserving. Identity-map is applied.
* Warning : Instrument-map was not trace preserving. Identity-map is applied.
* Warning : CPTP-map was not trace preserving. Identity-map is applied.
* Warning : Instrument-map was not trace preserving. Identity-map is applied.
.* Warning : CPTP-map was not trace preserving. Identity-map is applied.
* Warning : Instrument-map was not trace preserving. Identity-map is applied.
* Warning : CPTP-map was not trace preserving. Identity-map is applied.
* Warning : Instrument-map was not trace preserving. Identity-map is applied.
............E
======================================================================
ERROR: test_convert_openfermion_op (__main__.TestUtils)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/qulacs-osaka/python/test/test_qulacs.py", line 468, in test_convert_openfermion_op
    from openfermion import QubitOperator
ModuleNotFoundError: No module named 'openfermion'

----------------------------------------------------------------------
Ran 25 tests in 0.017s

FAILED (errors=1)
make[3]: *** [CMakeFiles/pythontest.dir/build.make:58: CMakeFiles/pythontest] Error 1
make[2]: *** [CMakeFiles/Makefile2:418: CMakeFiles/pythontest.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:425: CMakeFiles/pythontest.dir/rule] Error 2
make: *** [Makefile:164: pythontest] Error 2

ikanago commented 2 years ago

No module named 'openfermion' とあるので pip install openfermion を試してみてください．また，VSCode を使っている場合は .devcontainer/ に devcontainer の設定が入っていて，こちらのほうが便利かもしれないです． devcontainer を使うと，Docker の実行環境さえあればコーディングからテストまでを VSCode を介してコンテナ内で行えます．

forest1040 commented 2 years ago

ありがとうございます！ openfermion インストールしたら make pythontestも全てOKになりました。 .devcontainer/見てみます！

forest1040 commented 2 years ago

clang-formatでエラーになっているようですね。。確認します。

forest1040 commented 2 years ago

vscodeのformaterで、clang-formatをかけました。

WATLE commented 2 years ago

見ました。 RcPI = gate::PauliRotation(gate_now->get_target_index_list(), pauli_gate_now->get_pauli()->get_pauli_id_list(), M_PI); でも一見いいように見えるけど、キャストするのには理由がありますか?(試してません)

test/vqcsim/test_backprop.cppにbackpropのテストがあるので、それを参考に、パウリ回転行列のbackpropのテストもあるとありがたいです

WATLE commented 2 years ago

全体的方針はあってそうです

Hiroya-W commented 2 years ago

@forest1040 PRありがとうございます！

vscodeのformaterで、clang-formatをかけました。

CIの実行結果についてフィードバックが遅くなって申し訳ありません。また、formatの対応、ありがとうございました。 CIが落ちていたのは、https://github.com/Qulacs-Osaka/qulacs-osaka/issues/238 の問題の影響もありました。現在、devブランチに修正がマージされているので、feature-backpropブランチにも取り込んで push していただけますでしょうか。その上で、CIを再度実行してみます。

kotamanegi commented 2 years ago

@forest1040 PRありがとうございます！

ClsParametricPauliRotationGateにget_pauli()関数があるのは(場違い的な)違和感がないですし、便利な機能だと思います。位置についても、ClsParametricPauliRotationGate側がPauliゲートに関する情報を握っているのでこのクラスにget_pauli関数を追加する方針で良いと思います:+1:

他のコメントでも触れられているとおり、テストが不足しているのは事実なのでその部分を追加していただければ完璧だと思います。テストがあると、qulacs-osakaに関する他のソースコードの変更時に変な処理を追加してしまい、その影響で間違った動作を起こしてしまう(リグレッション)という事故の危険性を下げることができます qulacs-osakaではテストにgtestというライブラリを使っています。使い方はtest内のコードを眺めていただけるとだいたい理解できるかと思いますが、困ったときには公式ガイドを参照したり、ご質問いただけると幸いです。

forest1040 commented 2 years ago

@Hiroya-W 確認ありがとうございます。

現在、devブランチに修正がマージされているので、feature-backpropブランチにも取り込んで push していただけますでしょうか。

devブランチマージして push しました。確認をお願いします！

@kotamanegi 確認ありがとうございます。テストコードわかりました！週末にでも追加しておきます！もしアレでしたら先にマージして頂ければと思います。

forest1040 commented 2 years ago

@WATLE ありがとうございます。すみません！コメント見落としてました！

RcPI = gate::PauliRotation(gate_now->get_target_index_list(), pauli_gate_now->get_pauli()->get_pauli_id_list(), M_PI); でも一見いいように見えるけど、キャストするのには理由がありますか?(試してません)

get_pauli()は、ClsParametricPauliRotationGateクラスに追加しているため、キャストしています。

test/vqcsim/test_backprop.cppにbackpropのテストがあるので、それを参考に、パウリ回転行列のbackpropのテストもあるとありがたいです

了解です！テストコード入れときます。

forest1040 commented 2 years ago

@WATLE テストコードを追加しました。

https://github.com/forest1040/qulacs-osaka/commit/640e8bd595e07cb85a0b42ac9f9f4633112e62dd 確認をお願いします！

以下メモ特定テストコードだけを実行したい場合

cd build
make vqcsim_test
../bin/vqcsim_test --gtest_filter=Backprop*

WATLE commented 2 years ago

Looks Good To Meです👍

kosukemtr commented 2 years ago

https://github.com/Qulacs-Osaka/scikit-qulacs/pull/143 に関わる変更なのでマージしても大丈夫そうでしたらマージしておいていただけるとありがたいです！

kotamanegi commented 2 years ago

@forest1040 できればプルリクを出したforest1040さんの方でマージしていただけると嬉しいです！

forest1040 commented 2 years ago

すみません！私はマージ権限がないような気がします。。

kotamanegi commented 2 years ago

了解です、こちらの方でマージしておきます！ご協力ありがとうございました！

Qulacs-Osaka / qulacs-osaka

ParametricPauliRotationゲートをbackprop対象にする #237