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

added caiman advanced parameters #300

Merged
merged 6 commits into from
Mar 5, 2024

Conversation

itutu-tienday
Copy link
Collaborator

@itutu-tienday itutu-tienday commented Feb 20, 2024

@itutu-tienday itutu-tienday linked an issue Feb 20, 2024 that may be closed by this pull request
@itutu-tienday
Copy link
Collaborator Author

@ReiHashimoto
当件、reviewをお願いします。

Copy link

@ReiHashimoto ReiHashimoto left a comment

Choose a reason for hiding this comment

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

実装内容について、以下の開発視点ではOKです。

  • パラメータを格納しているops(CNMFParamsクラスのインスタンス)で、今回追加されたセクションの変数への変更が適用されている
  • ワークフローが正常に実行できる

一方で、以下の観点についてはユーザーレビューも必要と思われますので、現状ではmergeは保留できればと考えています。

  • 旧バージョンで実行していたcnmf, cnmf-eを含むワークフローについてはRUNができない
  • セクションの分け方がユーザビリティ上妥当であるか

@itutu-tienday
Copy link
Collaborator Author

@ReiHashimoto
一点メモですが、

旧バージョンで実行していたcnmf, cnmf-eを含むワークフローについてはRUNができない

上記への対処方法は、検討が必要そうですね。(caiman以外のNodeでも同様なため)
なお caimanなどを参考すると、パラメータの下位互換性の維持に、色々対策しているようです。
→ 基本的な対策は「1.極力パラメータ名は維持 or 2.新旧のパラメータ名のマッピングを用意」であるため、 初期のパラメータ名称設計が重要

@itutu-tienday
Copy link
Collaborator Author

@ReiHashimoto
当PRをアップデートしています。レビューお願いします。

image

Copy link

@ReiHashimoto ReiHashimoto left a comment

Choose a reason for hiding this comment

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

実装ありがとうございます。
わかりやすい構成になっていると思います。
動作も問題なさそうですね。

@itutu-tienday itutu-tienday merged commit f2fec3c into develop-main Mar 5, 2024
5 checks passed
@ReiHashimoto ReiHashimoto deleted the feature/caiman-additional-params branch April 11, 2024 08:17
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.

[123] CaImAnのパラメータの整理・追加
2 participants