[[OpenCVのバグ(trac)]]

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


* Itroduction [#p200d0e4]
-First, always check the https://github.com/opencv/opencv/wiki/How_to_contribute

* 時系列 [#q5b6e51d]
** testのfailに気づく [#h4d00b0d]
-ことの始まりは[[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であり&note{raspberry-pi-3:話題を呼んだラズパイ3は、OSは32bit。ODROID-C2は64bitのUbuntu16.04が動くのが特徴};、このテストをfixしてみようと気づいた
 
** エラーしている箇所を眺めてみる [#qf81f66d]
-内容は、11.054862976074219になるべき結果が、
-なぜか、11.05486''3929748535''になっているというエラー
-もともとも、HW命令を使った計算では、[[ニュートン法>メモ#f9155386]]で&mimetex(\frac{1}{\sqrt{x}});を求めて、そこから&mimetex(\sqrt{x});を求めるアプローチ
-反復することで、精度があがるので、まぁ、もう1回ぐらい計算を繰り返したら微妙な誤差も消えるだろう、と思って、試しに、もう1回繰り返してみた。
#geshi(c++){{
     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に入る値がどんどん&mimetex(\frac{1}{\sqrt{x}});に近づいていくという寸法。
-ちなみに、ArmのNEON命令を使っているので、float4つ分が一気に計算される。
-ベクタの要素のうち1つが、計算結果がズレ過ぎている、としてFAILEDになっていた

** 修正案を実装してPR(Pull Request)する [#n6a10b6c]
-試しに実装したらテストのFAILEDがPASSした
-なので、1行だけの修正だし、これで良かれと思って、PRを投げることにした
*** How to contributeページを読む [#q4e09cbe]
-ここからが大事なところ。OpenCVでは、[[How to contirbute>https://github.com/opencv/opencv/wiki/How_to_contribute]]&note{how-to-contribute-opencv:[[How to contirbute>https://github.com/opencv/opencv/wiki/How_to_contribute]], 2022-05-20 18:53版, 2022-07-07閲覧};というページがある
-ここでは、PRを投げる前に読むべき大事な点がたくさん書かれているので、本当に読んだ方が良い
-Gitのお作法だけでなく、コーディングスタイルに関しても書かれている。
-そして、なるだけ、機械的なチェックをパスするように指導されている。その一部をここで紹介
-''注意'' ここで書いている情報は2016-05-16時点の情報なので、これを読んでPRを投げる人は、必ずWikiで最新版の情報を参照して下さい
*** Githubのお作法 [#h46e1c5b]
-この節だけはOpenCV以外のプロジェクトにも共通する、gitのfork-mergeに関するお話&note{github-creating-pull-request:[[Creating a pull request - User Documentation>https://help.github.com/articles/creating-a-pull-request/]], 2016-05-16閲覧};&note{github-fork-a-repo:[[Fork A Repo>https://help.github.com/articles/fork-a-repo/]], 2016-05-16閲覧};
-手順としては
#ref(000-fork-and-pull-request.png)
+opencv/opencv リポジトリを自身のリポジトリに fork する
+自身のリポジトリから、ローカルに clone する
+ローカルでfixして、commitする
+自身のリポジトリにpushする
+GithubのインタフェースからPull Request を作る
-という感じです。詳細は[[githubの説明(リポジトリをforkする)>https://help.github.com/articles/fork-a-repo/]][[githubの説明(Pull Requestを使う)>https://help.github.com/articles/using-pull-requests/]]当たりを参照して下さい
*** 使用するブランチ [#q2b90933]
-以降の節は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 [#c409f59e]
-前述の手順でローカルにリポジトリを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を書くコツ [#h311d670]
-最近、PRを書く際に、テンプレートが表示されるようになりました。
-テンプレートには、大きく、**What does this PR change **という節を書くようになっています
-ここに、分かりやすく
--このPRで何をしたいのか
--このPRを何故書いたのか
--などを分かりやすく説明します
-私のオススメとしては、この説明文を上手く書けないようなPRは、恐らく細切れなPRに分割した方が良いと思います

**BuildBot [#eabbfe16]
-PRを投げたあとの全体的な流れは[[この図>http://code.opencv.org/projects/opencv/wiki/How_to_contribute/#Here-is-the-flow-chart-of-the-process]]が参考になると思います。
-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 [#cd975f57]
-Reviewのためには、Intelのメンバが承認することが必要です。
-マージする前に直すべき点、コードの書き方、テストが抜けている、など様々な指摘が来ますので、真摯に直しましょう。
-ReviewでOKが出ると、Intelのメンバがアサインされます
-アサインされたメンバが絵文字のサムアップ&note{thumb-up:どうもこのサムアップの絵文字はgithub公式のものらしくて、':+1'と書くと自動的に絵文字に変換されるらしい};を書くと、自動的にリポジトリにマージされます
-当然英語でやり取りをする必要があります。頑張りましょう。
***Reviewでの指摘 [#c7293d64]
-私の修正は、自分自身でも、イマイチピンと来なかったのですが、指摘で
--ニュートン法を繰り返すのはオーバースペック
--11.054863929748535と11.054862976074219はhex表現で0x4130E0B9と0x4130E0B''9''で、そもそも1ULPSしか違わない
--そんなbit exactに計算結果を比較しているのがナンセンス
--そして、よく見れば等しいかどうかの比較において、doubleとして比較されていたのが原因
--という指摘がありました。
-私自身、PRを投げた経験と同じぐらい、この指摘で多くのことを学びました。
-つまり問題の根源は計算でなく、floatの比較にEXPECT''_DOUBLE_''EQというマクロを使ってることでした。
-なので、そこを適切に直して、私のPRは無事マージされました
-ちなみにEXPECT_FLOAT_EQ内では、hex表現のfloatが、いくつ離れているか(ULPS)という点でtrueかfalseを返します。
-ちなみに、このhex表現のfloatがいくつ離れているか、というのは[[ULP>https://en.wikipedia.org/wiki/Unit_in_the_last_place]]という単位で表されます。
-丸め誤差を考慮し、最大でULP4つ分まで離れていても同じ浮動小数点である、と判定する仕組みになっています。

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


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


*過去に投げたOpenCVのPull Request [#v68f1c35]
[[Pull Requests · opencv/opencv>https://github.com/opencv/opencv/pulls?utf8=%E2%9C%93&q=is%3Apr+author%3Atomoaki0705]]

[[:OpenCV]]

トップ   編集 差分 履歴 添付 複製 名前変更 リロード   新規 一覧 検索 最終更新   ヘルプ   最終更新のRSS