Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#129_欠損対応(重み付き)TSOM3の追加 #130

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

forusufia
Copy link
Contributor

@forusufia forusufia commented Mar 14, 2020

** Description 説明 **
重み付き版のTSOMアルゴリズムで tsom3_weighted.py を追加しました

close #129

** Type of change 変更の種類**

  • New feature (non-breaking change which adds functionality)

** How Has This Been Tested? どのようにテストしたか?**
somf\tests\tsom\tsom3_weighted\tsom3_weighted.py とのペアプロをお願い致します。

  • 数式から重み付きのTSOM3 をプログラム化
  • somf\tests\tsom\tsom3_weighted\tsom3_weighted.py と allclose

ペアプロ用に
somf\tests\tsom\tsom3_weighted\allclose_tsom3_weighted_harada_vs_another.py
もありますので、適宜モデルの呼び出し部分をご自分のプログラムに書き換えて活用ください。

@forusufia forusufia added the enhancement New feature or request label Mar 14, 2020
@forusufia forusufia self-assigned this Mar 14, 2020
@forusufia
Copy link
Contributor Author

数式再掲

tsom3_1

tsom3_2

@takuro-Ishida takuro-Ishida self-requested a review March 15, 2020 17:14
@ae14watanabe
Copy link
Member

これ、somfの既存のクラスに機能追加という形で実現はできないでしょうか?

@ae14watanabe
Copy link
Member

メンション飛ばし忘れた @forusufia
いやね、ちょっとあまりにもtsom関係のクラスが多すぎるなと思って( #128 で問題提起はしてる)

@forusufia
Copy link
Contributor Author

@ae14watanabe
クラス呼び出しの時の引数を一個増やしてモード切替するような感じでしょうか?
weight = "enable" or "disable" みたいな

@forusufia forusufia requested a review from noguchikazuki March 21, 2020 07:47
@ae14watanabe
Copy link
Member

ae14watanabe commented Mar 22, 2020

いや、重み(欠損の場合は2値が入ってるマスク?)を渡したらそれ使うし、渡さなかったら使わないみたいな仕様でいいんじゃないでしょうか?TSOM2 #101 の仕様と合わせると良さそう。

@ae14watanabe
Copy link
Member

ae14watanabe commented Mar 22, 2020

tsom3_weighted.py って tsom3.py を自分でコピーしてそれを修正してる感じですか?もしそうなら差分をtsom3.pyに反映させてプルリク出すと良さそう。
完全に1からフルスクラッチしてるなら渡辺の提案は新しい作業することになるのでちょっと面倒そうですね。

@takuro-Ishida
Copy link
Member

おそらくtsom3.pyをコピーしてやってる感じだと思う.

とりあえず,僕は原田くんがいう通り実装してallcloseで一致するか確かめます

@ae14watanabe
Copy link
Member

マージするなら機能追加って感じだと思ってて、それは追い追いやって欲しいけど、めっちゃ急ぎってわけでもないので余裕のある時に @forusufia にはやってもらえばいいかなと思う。
@takuro-Ishida がペアプロ用に先行してクラスを上げるのは良さそうですね。 @forusufia 自身の研究の作業にも必要だと思うので。

@forusufia
Copy link
Contributor Author

わかりました。余裕あるときに機能追加の形に直します!

@forusufia
Copy link
Contributor Author

@takuro-Ishida
ひとまず一つ差異がありました

@forusufia のtsom3
H1 = np.exp(-*distance1 / (2 * pow(sigma1, 2)))

@takuro-Ishida のtsom3
H1=np.exp(-0.5*Dist1/(2 * pow(sigma1, 2)))

H2, H3 も同様で、Dist の前に0.5 が掛かっているかいないかが異なります
この0.5って分母の2で割るのが2重になっていませんか?

@takuro-Ishida
Copy link
Member

takuro-Ishida commented Apr 10, 2020

H2, H3 も同様で、Dist の前に0.5 が掛かっているかいないかが異なります
この0.5って分母の2で割るのが2重になっていませんか?

なるほど.
そこは自分が間違ってますね.

ありがとうございます

@forusufia
Copy link
Contributor Author

エポック5回目でk3 の値が違ってきて
14
エポック6回目からY などに影響が出始めてるのがわかるのですが
15

@takuro-Ishida @noguchikazuki
延々デバッグしてますが手詰まり感があるので、ちょっと別の人の視点が欲しいです
お助けください...

@forusufia
Copy link
Contributor Author

@takuro-Ishida
めちゃくちゃシンプルに見落としてました
tsom3_weighted_ishida.py の勝者決定のところでG が掛かってなかったみたいです・・・

16

修正したら一応一致してるっぽいです
17

チェックお願いします~

@forusufia forusufia requested a review from takuro-Ishida April 14, 2020 04:02
@takuro-Ishida
Copy link
Member

@forusufia
了解です!
自分も検証して確かめてみます!

@takuro-Ishida
Copy link
Member

結果的に俺がひたすら間違ってたって感じか...

申し訳ないです

@forusufia
Copy link
Contributor Author

forusufia commented Apr 14, 2020

いえいえ~ 
途中の計算など色々と見直ししたので
助かっています

@ae14watanabe
Copy link
Member

ae14watanabe commented Apr 14, 2020

@forusufia お疲れ様です!
確認なんやけど、***.npyみたいなファイルってデバッグ用よね?
gitは一旦commitしちゃうと、ファイルを削除しても「ファイルを削除した」というコミットが残るだけで内部ではファイルを保存してるので、こういうファイルはコミットしない方が吉な気がします。リポジトリの容量が増えていってしまうので…

@takuro-Ishida
Copy link
Member

@ae14watanabe
リポジトリの容量って有限?

@forusufia
Copy link
Contributor Author

@ae14watanabe
デバッグ用です!
間違えて追加しちゃってたみたいですね・・・気を付けます

Copy link
Member

@takuro-Ishida takuro-Ishida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestしてみてallcloseでTrueになったことに確認しました
Approveしました
image

@ae14watanabe
Copy link
Member

@forusufia できれば消せませんか?ちょっとめんどくさそうだけど

@forusufia
Copy link
Contributor Author

@ae14watanabe
ファイルは消しましたが、コミット自体をですか?

@forusufia
Copy link
Contributor Author

プログラムをいったん退避して戻ればいいんでしょうか

@ae14watanabe
Copy link
Member

@forusufia うん、ファイルを追加したという事実を消さないと、表面的にファイルを削除しても.gitの中に残ってるんですよファイルが。それがリポジトリの容量を増やしてしまうので。

@ae14watanabe
Copy link
Member

@takuro-Ishida リポジトリの容量自体は有限じゃないけど、クローンする人がだるいよねってだけ。まぁ別にいいかこれぐらい。

@ae14watanabe
Copy link
Member

やっぱめんどくさいね。やめとこう。

@ae14watanabe
Copy link
Member

ae14watanabe commented Apr 15, 2020

あと別件ですけど、reviewでapproveにするのはTSOMへの機能追加という形で実装できた時まで待った方がいいと思います。これでマージしてもtestにしか反映されないので…

@takuro-Ishida
Copy link
Member

takuro-Ishida commented Apr 15, 2020

reviewでapproveにするのはTSOMへの機能追加という形で実装できた時まで待った方がいいと思います。

確かに.
model/tsom3/tsom3.pyに統合するのが一番ですが,それが大変なのであれば
model/tsom3の中にtsom3_weighted.pyを移植するでもいいと個人的には思います.

@takuro-Ishida takuro-Ishida self-requested a review April 15, 2020 13:27
@ae14watanabe
Copy link
Member

@takuro-Ishida @forusufia
今回の重み付きって普通のTSOMの一般化(普通のTSOMは重みが全て1という特殊な状況)って認識で合ってますか?そうじゃなくてアルゴリズムがかなり違うなら別のクラスとして用意するのもアリかなと思って。わざわざ別のクラスとして用意してまで焦ってマージする必要もないと個人的には思ってます。

@takuro-Ishida
Copy link
Member

今回の重み付きって普通のTSOMの一般化(普通のTSOMは重みが全て1という特殊な状況)って認識で合ってますか?

あってます.当面は欠損データを扱う予定です.

わざわざ別のクラスとして用意してまで焦ってマージする必要もないと個人的には思ってます。

そこは,Assignees(@forusufia )次第かなと.
緊急で使う必要があるのであれば,ひとまず別クラスで入れてしまうのもいいのかなとは思ってます.(リポジトリの容量は圧迫してしまいますが...)
おっしゃる通りtsom3.pyに機能追加という形で入れるのが一番ですが.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

欠損対応(重み付き)Tensor SOM3 の追加
4 participants