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

Remove: OjtPhoneme 単純化 #829

Closed
3 tasks done
tarepan opened this issue Dec 7, 2023 · 2 comments · Fixed by #846
Closed
3 tasks done

Remove: OjtPhoneme 単純化 #829

tarepan opened this issue Dec 7, 2023 · 2 comments · Fixed by #846

Comments

@tarepan
Copy link
Contributor

tarepan commented Dec 7, 2023

内容

要望: OjtPhoneme クラス変数と一部テストの廃止による単純化

現在の OjtPhoneme は以下のクラス変数を持つ:

  • phoneme_list
  • num_phoneme
  • space_phoneme

これら3属性はコアへ渡される特徴量の形式を決定する属性であり、決して変更されてはならない。
しかし現在の OjtPhoneme はこれら3属性をパブリッククラス変数として公開しており、意図しない変更によるバグが起きうる。

よって、1点目として、phoneme_list / num_phoneme / space_phoneme のプライベート変数化を提案します。

またこれら3属性に関わるテストが存在する。
その一部は(決して変更されない)3属性の値をテストしている。

この背景に基づいて次の質問があります:

Q: これら定数テストはどのような意図でしょうか?

def test_phoneme_list(self):
self.assertEqual(OjtPhoneme.phoneme_list[1], "A")
self.assertEqual(OjtPhoneme.phoneme_list[14], "e")
self.assertEqual(OjtPhoneme.phoneme_list[26], "m")
self.assertEqual(OjtPhoneme.phoneme_list[38], "ts")
self.assertEqual(OjtPhoneme.phoneme_list[41], "v")

def test_const(self):
TRUE_NUM_PHONEME = 45
self.assertEqual(OjtPhoneme.num_phoneme, TRUE_NUM_PHONEME)
self.assertEqual(OjtPhoneme.space_phoneme, "pau")

もし当初の意図が現在は自然と達成されているなら、これらテストの廃止による単純化を提案します。

Pros 良くなる点

  • 潜在的バグ源の事前潰し
  • テスト単純化によるメンテコスト低減

Cons 悪くなる点

  • 無し

実現方法

  • phoneme_list: モジュール定数 _PHONEME_LIST に切り出し、__init__() 内でプライベート変数に紐付け
  • num_phoneme: モジュール定数 _NUM_PHONEME に切り出し、__init__() 内でプライベート変数に紐付け
  • __init__() 内の文字列化、変数として廃止

VOICEVOXのバージョン

0.14.10

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux
@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 9, 2023

プライベート変数化、良いと思います!!

Q: これら定数テストはどのような意図でしょうか?

これらの値が変わっても他の全てのテストは通るかもしれませんが、実際に音声合成しようとするとコアとの兼ね合いで正しく実行できないので、コード変更によりこれらの値が変わってないことを保証するテストなのかなと思いました!
テスト実装に明るくなくて、このようなテストが一般的なのかどうかちょっと自信ないです・・・!

@tarepan
Copy link
Contributor Author

tarepan commented Dec 9, 2023

プライベート変数化、良いと思います!!

👍

コード変更によりこれらの値が変わってないことを保証するテスト

開発者が phoneme_list 等の変更をcommitしていないことの保証ですね、なるほどです。
であればテストには引き続き頑張ってもらう(廃止無し)方向でPRを作ります。

依存PR一覧

以下のIssues/PRsに依存しているため、これら解決後に着手します: 追記@2023-12-10T15:10 先行着手しました。

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

Successfully merging a pull request may close this issue.

2 participants