OpenCVのバグ(trac)

この度、OpenCVへのPull Request がマージされたので、その時の記録とかを書いておきます。

Itroduction

時系列

testのfailに気づく

  • ことの始まりはODROID-C2を買って、その上でOpenCVのtestを走らせたことから
  • 何故か、testの一部がFAILEDとなる
  • 見てみると、hal_intrinというテストが失敗していた
  • 具体的には、HW命令を使用して平方根(square root)を求めた結果と、std::sqrtの結果が一致しないというエラーだった
    /home/odroid/opencv-fork/modules/core/test/test_intrin.cpp:578: Failure
    Value of: resF[i]
      Actual: 11.054863929748535
    Expected: std::sqrt(data1[i]*data1[i] + data2[i]*data2[i])
    Which is: 11.054862976074219
  • ODROID-C2は、純粋に64bitArmであり*1、このテストをfixしてみようと気づいた

エラーしている箇所を眺めてみる

  • 内容は、11.054862976074219になるべき結果が、
  • なぜか、11.054863929748535になっているというエラー
  • もともとも、HW命令を使った計算では、ニュートン法\frac{1}{\sqrt{x}}を求めて、そこから\sqrt{x}を求めるアプローチ
  • 反復することで、精度があがるので、まぁ、もう1回ぐらい計算を繰り返したら微妙な誤差も消えるだろう、と思って、試しに、もう1回繰り返してみた。
         float32x4_t e = vrsqrteq_f32(x1);
         e = vmulq_f32(vrsqrtsq_f32(vmulq_f32(x1, e), e), e);
         e = vmulq_f32(vrsqrtsq_f32(vmulq_f32(x1, e), e), e);
    +    e = vmulq_f32(vrsqrtsq_f32(vmulq_f32(x1, e), e), e);
         return v_float32x4(vmulq_f32(x.val, e));
     }
  • こんな感じに、eに入る値がどんどん\frac{1}{\sqrt{x}}に近づいていくという寸法。
  • ちなみに、ArmのNEON命令を使っているので、float4つ分が一気に計算される。
  • ベクタの要素のうち1つが、計算結果がズレ過ぎている、としてFAILEDになっていた

修正案を実装してPR(Pull Request)する

  • 試しに実装したらテストのFAILEDがPASSした
  • なので、1行だけの修正だし、これで良かれと思って、PRを投げることにした

How to contributeページを読む

  • ここからが大事なところ。OpenCVでは、How to contirbute*2というページがある
  • ここでは、PRを投げる前に読むべき大事な点がたくさん書かれているので、本当に読んだ方が良い
  • Gitのお作法だけでなく、コーディングスタイルに関しても書かれている。
  • そして、なるだけ、機械的なチェックをパスするように指導されている。その一部をここで紹介
  • 注意 ここで書いている情報は2016-05-16時点の情報なので、これを呼んでPRを投げる人は、必ずWikiで最新版の情報を参照して下さい

Githubのお作法

  • この節だけはOpenCV以外のプロジェクトにも共通する、gitのfork-mergeに関するお話*3*4
  • 手順としては
    000-fork-and-pull-request.png
  1. opencv/opencv リポジトリを自身のリポジトリに fork する
  2. 自身のリポジトリから、ローカルに clone する
  3. ローカルでfixして、commitする
  4. 自身のリポジトリにpushする
  5. GithubのインタフェースからPull Request を作る

使用するブランチ

  • 以降の節はOpenCVに限った話
  • OpenCVは2016年5月16日現在、3.0系列と2.4系列の2本の系列があります。
    • OpenCV2.4系列は既に「新機能の追加は行わない」とアナウンスされており、bug fixだけを取り込むこととなっています
    • OpenCV3.0系列がメインの開発対象となっており、新規機能はこちらに取り込むこととなっています
  • なので、bug fix ならば2.4系列、もしくは両方に、新規機能の実装ならば3.0系列に、それぞれPRを投げることになります。
  • 今回私が投げたPRはbug fixなので、本来は2.4系列にも投げるべきなのですが、そもそもhal_intrinがHALモジュール(Hardware Acceleration Layer)という、3.0から入った機能を使った話なので、今回はmaster(3.0系列)にPRを投げることにしました。

pre-commit hook

  • 前述の手順でローカルにリポジトリをcloneした後、作業をする前に、pre-commit hook を用意すると、みんな幸せになれます
  • 具体的には、cloneしたリポジトリの .git/hooks/pre-commit.sample というファイルを、 .git/hooks/pre-commit にリネームします。
  • この.gitディレクトリは通常隠しディレクトリなので、うまいこと、やってください
  • これをすることで、White spce に関する規約をcommit 直前にgitがチェックしてくれます。こうすることで、自然とすべてのPRがWhite spaceの規約に則ることになります。
    • ちなみに、OpenCVはインデントはスペース4つです。タブは使いません。
  • 例えばインデントにタブを使うと、Commit自体がエラーになります

PRを書くコツ

  • 最近、PRを書く際に、テンプレートが表示されるようになりました。
  • テンプレートには、大きく、**What does this PR change **という節を書くようになっています
  • ここに、分かりやすく
    • このPRで何をしたいのか
    • このPRを何故書いたのか
    • などを分かりやすく説明します
  • 私のオススメとしては、この説明文を上手く書けないようなPRは、恐らく細切れなPRに分割した方が良いと思います

BuildBot

  • PRを投げたあとの全体的な流れはこの図が参考になると思います。
  • PRを投げたあとは、機械テストが実行されます。
  • 2016年5月16日時点では
    • Linux x64ビルド
    • Win7 x64ビルド(VS2013)
    • Win10 x64ビルド(VS2015)
    • Mac
    • Android armeabi-v7a
    • OpenCL
    • Linux x64 Debug
    • Docs
    • iOS
    • の9種類のビルド+テストが実行されます
  • オプショナルに
    • OpenCL Intel
    • Win32
    • Arm
    • Android pack
    • などが実行されますが、実行される条件は不明です。
  • この機械テストは、自分のローカルで再現可能です。そのことに関してはまたその内書きます
  • 全ての自動ビルドに、Warningもなく、エラーもなく、その上で全てのテストがpassすれば、晴れてマージ可能となります。
  • ただし、マージされるかどうかは、Reviewが入ります

Review

  • Reviewのためには、Intelのメンバが承認することが必要です。
  • マージする前に直すべき点、コードの書き方、テストが抜けている、など様々な指摘が来ますので、真摯に直しましょう。
  • ReviewでOKが出ると、Intelのメンバがアサインされます
  • アサインされたメンバが絵文字のサムアップ*5を書くと、自動的にリポジトリにマージされます
  • 当然英語でやり取りをする必要があります。頑張りましょう。

Reviewでの指摘

  • 私の修正は、自分自身でも、イマイチピンと来なかったのですが、指摘で
    • ニュートン法を繰り返すのはオーバースペック
    • 11.054863929748535と11.054862976074219はhex表現で0x4130E0B9と0x4130E0B9で、そもそも1ULPSしか違わない
    • そんなbit exactに計算結果を比較しているのがナンセンス
    • そして、よく見れば等しいかどうかの比較において、doubleとして比較されていたのが原因
    • という指摘がありました。
  • 私自身、PRを投げた経験と同じぐらい、この指摘が的確でした。
  • つまり問題の根源は計算でなく、floatの比較にEXPECT_DOUBLE_EQというマクロを使ってることでした。
  • なので、そこを適切に直して、私のPRは無事マージされました
  • ちなみにEXPECT_FLOAT_EQ内では、hex表現のfloatが、いくつ離れているか(ULPS)という点でtrueかfalseを返します。
  • ちなみに、このhex表現のfloatがいくつ離れているか、というのはULPという単位で表されます。
  • 丸め誤差を考慮し、最大でULP4つ分まで離れていても同じ浮動小数点である、と判定する仕組みになっています。

マージ(Squash)

  • Reviewや、BuildBotでの修正の過程で複数のcommitがpushされることがあります
  • 通常、その過程を全て1つのcommitにまとめる(Squashする)ことが要求されます
    • branch元にgit reset --mixedする
    • pushした先のbranchをmasterにmergeする。その際、--squashオプションを付ける
    • 再度commitする
    • 再度pushする。この際、SHA1値が変わってるので、push --forceする

感想

  • テスト、すげー。そして大事
  • 一発でテストに通ると思わないほうが良い。テストでfailしたら恥ずかしいと思うけれど、恥じずに直す
  • 何が問題なのか、ちゃんと説明できないなら、良く精査した方が良い

過去に投げたOpenCVのPull Request

Pull Requests · opencv/opencv

:OpenCV


*1  話題を呼んだラズパイ3は、OSは32bit。ODROID-C2は64bitのUbuntu16.04が動くのが特徴
*2  How to contirbute, 2014-07-14 18:01版, 2016-05-16閲覧
*3  Creating a pull request - User Documentation, 2016-05-16閲覧
*4  Fork A Repo, 2016-05-16閲覧
*5  どうもこのサムアップの絵文字はgithub公式のものらしくて、':+1'と書くと自動的に絵文字に変換されるらしい

添付ファイル: file000-fork-and-pull-request.png 140件 [詳細] file006-pullrequest-github.png 72件 [詳細] file005-pullrequest.png 72件 [詳細] file004-pullrequest.png 65件 [詳細] file003-push.png 70件 [詳細] file002-modify.png 70件 [詳細] file001-clone.png 74件 [詳細] file000-fork.png 78件 [詳細]

トップ   編集 凍結 差分 バックアップ 添付 複製 名前変更 リロード   新規 一覧 単語検索 最終更新   ヘルプ   最終更新のRSS
Last-modified: 2017-08-24 (木) 09:55:32 (93d)