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

Add implementation of K-SVD #7

Merged
merged 8 commits into from
Mar 30, 2018
Merged

Add implementation of K-SVD #7

merged 8 commits into from
Mar 30, 2018

Conversation

sutkss
Copy link
Collaborator

@sutkss sutkss commented Mar 28, 2018

No description provided.

@@ -1 +1,3 @@
numpy
scipy
scikit-learn>=0.19.0
Copy link
Contributor

@tksmd tksmd Mar 28, 2018

Choose a reason for hiding this comment

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

sklearn の特定の Mixin に対する明な依存関係が出てきたので、念のためバージョン追加しておきました。

# initialize dictionary
dict_init = random_state.rand(n_components, n_features)

self.components_, code, self.error_, self.n_iter_ = _ksvd(
Copy link
Contributor

@tksmd tksmd Mar 28, 2018

Choose a reason for hiding this comment

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

インスタンス属性の名前 ( components_ とか ) を sklearn の慣例にあわせ、最後にアンダースコアを足しました。SparseCodingMixin はこの名前を期待して transform しているところがあるためです。


Y = Y.T
X = X.T
A = A.T
Copy link
Contributor

@tksmd tksmd Mar 28, 2018

Choose a reason for hiding this comment

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

ここの三つの転置なのですが、

  • そもそも code_init と dict_init が逆だった
  • コードコメントに書かれている行列の構造と、実装で期待している構造が逆だった

といった背景から足したもので、後続の k-SVD 本体の処理を書き換えるよりは簡単・安全そうにみえたのでこうしました。
が、あまり見目はよろしくないので、もしよければ綺麗にしてもらえると。。。

Copy link
Contributor

@tksmd tksmd left a comment

Choose a reason for hiding this comment

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

@sutkss 一通りレビュー + テストケース足しました。

コードに個別コメントした通り ksvd 実装の中で、NumPy の array の次元がメソッドのコメントに書かれているものと、内部の実装で期待している部分が逆転しているところがあったので、若干修正いれました。

まだ画像では試していませんが、テストでは fit と transform はできていることは確認してます。

ksvd の内部実装のところのミスマッチだけちょっと直したいなぁ、というのが正直なところですが、そこの部分の調整お願いできたりしますか?

@sutkss
Copy link
Collaborator Author

sutkss commented Mar 29, 2018

scikit learnの実装が
https://github.com/scikit-learn/scikit-learn/blob/a24c8b46/sklearn/decomposition/dict_learning.py#L170-L179
のように, X ~= code*dictionaryとなっているため, Y = AXであれば, Aがcode, Xがdictionaryに対応するのが正解です.

コメントの順番が間違えてました.

@tksmd
Copy link
Contributor

tksmd commented Mar 29, 2018

Y = AXであれば, Aがcode, Xがdictionaryに対応するのが正解です.

あー、そうか、そうでしたね。元のコードで transform した時に dictionary との次元があわなくて、その辺りを直すために変更したという背景もあったのですが、では整理すると、

  • 実装は元のままでよい
  • コメントの順 ( dictionary, code ) を変えないのなら、返り値の順を A, X ではなく、X, A にすればよい

ということで、あってます?であれば、僕の方でまた 3f2840b を revert して元に戻して試してみようと思います。 @sutkss

@sutkss
Copy link
Collaborator Author

sutkss commented Mar 29, 2018

@tksmd Y = WH = code * dictionaryとして, 書きなおしてみました.
テストも少しだけ変えてます.
レビューお願いします.

@tksmd tksmd changed the title Issue/6/ksvd Add implementation of K-SVD Mar 30, 2018
@tksmd tksmd merged commit 476a0a9 into development Mar 30, 2018
@tksmd
Copy link
Contributor

tksmd commented Mar 30, 2018

@sutkss 👍 です。僕の方でもテストのイテレーションを増やしてエラーが小さくなっているのを確認できるように書きかえてマージしました。

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

Successfully merging this pull request may close these issues.

2 participants