Qulacs-Osaka / qulacs-osaka

Development branch of qulacs at Osaka Univ
MIT License
13 stars 6 forks source link

refactor Parametric Gate/Cirecuit #347

Closed KowerKoint closed 2 years ago

KowerKoint commented 2 years ago

ParametricGateとParametricCircuitの実装を読んで気になったところを書き換えました

  1. QuantumGate_SingleParameterのコンストラクタの_gate_propertyのスイッチ、xorだともし継承して同じことをもう一回書いたら0になってしまうリスクがあるので意味的に正しいorにした
  2. QuantumGate_SignelParameterOneQubitRotationのコンストラクタで、親クラスのコンストラクタで行われていることが2回発生していてそうだったので削除した
  3. QuantumGate_SingleParameterOneQubitRotationは他のクラスで継承することを前提としているはずで、もしそのままインスタンス化して使われた場合にupdate_funcがNULLのときに困るので例外を追加した
  4. ParametricCircuitのcopy()でstd::find()を使ってるが、_parametric_gate_positionは必ず単調増加で、検索クエリも単調増加なためposをカーソルとしてO(gate_num)で探索するように変更した
  5. parameterをget/setする用途の関数での配列外参照がただのエラー出力になっていたので例外対応した

3はよく考えたらQuantumGate_SingleParameterOneQubitRotationが純粋仮想関数を持っていてインスタンス化できないので起こり得ない事故への対処になってました。 不要なら消します。

ikanago commented 2 years ago

4.について,ParametricQuantumCircuit::add_parametric_gate(QuantumGate_SingleParameter* gate, UINT index)index_parametric_gate_positionpush_back() している(parametric_circuit.cpp:L45)から _parametric_gate_position は単調増加にならないんじゃないかなと思って以下のコードを実行してみました.

#include <iostream>
#include "vqcsim/parametric_circuit.hpp"
#include "vqcsim/parametric_gate_factory.hpp"
#include "cppsim/circuit.hpp"

int main() {
    constexpr UINT n = 3;
    auto circuit = ParametricQuantumCircuit(n);
    circuit.add_parametric_RX_gate(0, 0.0);
    circuit.add_parametric_RX_gate(1, 0.0);
    circuit.add_parametric_RX_gate(1, 0.0);
    circuit.add_parametric_gate(gate::ParametricRX(2, 0.0), 1);
    for (UINT i = 0; i < 4; i++) {
        std::cerr << circuit.get_parametric_gate_position(i) << ", ";
    }
}

出力として

0, 2, 3, 2, 

が得られ,たしかに単調増加ではないことが確認できたのですが,本来は

0, 2, 3, 1, 

となるはずです. おそらく add_gate条件を ~val > index とすべきだと思います.~ こうしたほうがいいと思います:

void ParametricQuantumCircuit::add_gate(QuantumGateBase* gate, UINT index) {
    QuantumCircuit::add_gate(gate, index);
    for (UINT i = 0; i < this->_parametric_gate_position.size() - 1; i++) {
        if (this->_parametric_gate_position[i] >= index) {
            this->_parametric_gate_position[i]++;
        }
    }
}

というわけでこれを先に修正したほうがよさそうですが、自信がないので他のレビュアーのおふたりも確認お願いします 🙏

Hiroya-W commented 2 years ago

3はよく考えたらQuantumGate_SingleParameterOneQubitRotationが純粋仮想関数を持っていてインスタンス化できないので起こり得ない事故への対処になってました。 不要なら消します。

継承したクラスでも、コンストラクタで値の代入を忘れていれば_update_func関数の呼び出しでSegmentation faultしそうなので、ある方が適切だと思います!

Hiroya-W commented 2 years ago

ikanagoさんの

_parametric_gate_position は単調増加にならないんじゃないかな

は、その通りだと思いました。また、add_gateでindexが重複しないようにする際に、_parametric_gate_positionに追加した1もインクリメントされてしまっているんですね。提示してくれているコードで解決しそうです:+1:

add_gate関数でこのインデックスの調整が入るのも、良くない気がしますね。void ParametricQuantumCircuit::add_parametric_gate(QuantumGate_SingleParameter* gate, UINT index)で処理するようにして、

void ParametricQuantumCircuit::add_gate(QuantumGateBase* gate, UINT index) {
    QuantumCircuit::add_gate(gate, index);
}

であるべきかも?とも思いました。

codecov-commenter commented 2 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (dev@c38bc88). Click here to learn what that means. The diff coverage is n/a.

@@          Coverage Diff           @@
##             dev     #347   +/-   ##
======================================
  Coverage       ?   71.65%           
======================================
  Files          ?       84           
  Lines          ?     5976           
  Branches       ?        0           
======================================
  Hits           ?     4282           
  Misses         ?     1694           
  Partials       ?        0           

Continue to review full report at Codecov.

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

KowerKoint commented 2 years ago

add_gateに中途への挿入があるのを見落として単調増加だと勘違いしてました:bow:

KowerKoint commented 2 years ago

add_gate関数でこのインデックスの調整が入るのも、良くない

add_gate(gate, index) でも途中に入ったgateの文_parametric_gate_position が動きます。 add_gate での調整はもとのコードのままにして、 add_parametric_gate での _parametric_gate_position.push_back(index)add_gate() の呼び出しよりあとにすれば良さそうです。

KowerKoint commented 2 years ago

add_gate での調整はもとのコードのままにして、 add_parametric_gate での _parametric_gate_position.push_back(index) を add_gate() の呼び出しよりあとにする

一度この方針で実装してみました。add_parametric_gate_copy() のほうも ParametricQuantumCircuit::add_copy() を使うような実装にして統一しました。 これで合ってるか再度見直しお願いします。

KowerKoint commented 2 years ago

kotamanegi さんのレビューも反映しました :bow:

KowerKoint commented 2 years ago

a72f7b3 のコミットでGateApplyがセグフォで落ちるようになってしまって、原因を調査してるんですが未だにわかりません……

#include <iostream>

#include "cppsim/circuit.hpp"
#include "vqcsim/parametric_circuit.hpp"
#include "vqcsim/parametric_gate_factory.hpp"

int main() {
    const UINT n = 3;
    ParametricQuantumCircuit circuit(n);
    circuit.add_parametric_RX_gate(0, 0.1);
    QuantumState state(n);
    circuit.update_quantum_state(&state);
}

update_quantum_state のときに落ちます。 nが2以下の場合も落ちません。なぜ…

KowerKoint commented 2 years ago

9494a5d で条件分岐を壊してたのが本質でした。 失礼しました。:bow:

KowerKoint commented 2 years ago

テスト書きました

KowerKoint commented 2 years ago

macOSのテスト環境だと初期化してないメンバが0にはなってないようで補足できずにSegmentation Faultが起きてしまいました このような違いをなくすために一応NULLに初期化することにしました。 0以外の存在しないポインタは補足できませんが故意的でない限りそう起こらないのでほっておきます。