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

整理: TTS テスト入力をリアルにし警告を解消 #1297

Merged
merged 11 commits into from
Jun 26, 2024

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 22, 2024

内容

概要: TTS mock 出力をリアルにし警告を解消してリファクタリング

talk の一部に、全音素長を 0 とする非現実的な出力の mock があった。
これにより mock core 内の NumPy 計算が危険入力アリと判断し、テスト時に warning を出していた。

このような背景から、TTS mock 出力をリアルにし警告を解消してリファクタリングすることを提案します。

関連 Issue

無し

@tarepan tarepan requested a review from a team as a code owner May 22, 2024 05:21
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 22, 2024 05:21
@tarepan tarepan marked this pull request as draft May 22, 2024 05:27
@tarepan
Copy link
Contributor Author

tarepan commented Jun 16, 2024

MacOS でスナップショットの不一致が発生している。スナップショット値は Linux で作っているので、また数値精度系…?

@Hiroshiba
Copy link
Member

そんな雰囲気感じますね。。
一度ハッシュ化を解いて眺めてみるとなにかわかるかも…?

@tarepan tarepan marked this pull request as ready for review June 21, 2024 15:37
@tarepan
Copy link
Contributor Author

tarepan commented Jun 21, 2024

修正により CI pass しました。
レビューよろしくお願いします。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

タイトルですが、mock版TTSEngineの出力をリアルにしたのかなと思い、実装と違っていたので少し混乱しました・・・!

数値変更は良いと思うのですが、テストが通るようにデータを変えている点が少し気になりました。
テスト結果がOSごとに違うのであれば、もしかしたらresampleに誤差があるかも・・・?
サンプリングレートを24000にしてテストが通るなら、そうだと思います。

だとしてら、resampleをDIできる形にすると良いのかも・・・。

test/unit/tts_pipeline/test_tts_engine.py Outdated Show resolved Hide resolved
@tarepan tarepan changed the title 整理: TTS mock 出力をリアルにし警告を解消 整理: TTS テスト入力をリアルにし警告を解消 Jun 22, 2024
@tarepan
Copy link
Contributor Author

tarepan commented Jun 22, 2024

タイトルですが、mock版TTSEngineの出力をリアルにしたのかなと思い、実装と違っていたので少し混乱

👍️
妥当な指摘です。
リネームします。

テスト結果がOSごとに違うのであれば、もしかしたら resample に誤差があるかも・・・?

あくまでググった知識なのですが、NumPy は OS ごとの float 計算の差を吸収 しない ようです。単なる tanh でも計算結果が OS 間で異なるケースとかあるようです。
なので resample が原因の可能性もありますが、他のどこでも原因になりうると思われます。


@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re-review よろしくお願いします。

@Hiroshiba
Copy link
Member

Hiroshiba commented Jun 22, 2024

あくまでググった知識なのですが、NumPy は OS ごとの float 計算の差を吸収 しない ようです。単なる tanh でも計算結果が OS 間で異なるケースとかあるようです。

なーーーーーーるほどです!!! たしかに。

resampleはアルゴリズムによってかなり値がぶれそうなので、怪しいな~と思った次第でした。
まあでも上から5桁目がずれるのはresample以外でも起こりうる氣がしました!

@tarepan
Copy link
Contributor Author

tarepan commented Jun 25, 2024

#1426 に追従する log スケール化により、テストが通るようにデータを変えている の部分をリバートできました。


@Hiroshiba
全指摘箇所の反映・テストパスを確認しました。Re2-review よろしくお願いします。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

@tarepan
Copy link
Contributor Author

tarepan commented Jun 26, 2024

レビューありがとうございました、マージ可能です👍️

@Hiroshiba Hiroshiba merged commit 90123ba into VOICEVOX:master Jun 26, 2024
4 checks passed
@tarepan tarepan deleted the refactor/mock_output_tts branch June 26, 2024 06:00
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