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.

Sign up for GitHub

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

Jump to bottom

jlabelを導入する #742

Merged
merged 35 commits into from
Feb 26, 2024
Merged

jlabelを導入する #742

merged 35 commits into from
Feb 26, 2024

Conversation

phenylshima
Copy link
Contributor

@phenylshima phenylshima commented Feb 8, 2024 •
edited
Loading

内容

jlabelを導入します．

jlabelは，フルコンテキストラベルの構造(struct)と，シリアライザー・パーサーを含むライブラリです．
これを使うことによって，従来のf6といった表記ではなく，accent_phrase.accent_phrase_position_backwardのように意味を持った文字列で書けるため，可読性が向上することが期待されます．

また，このPRではVec<Label>から（Utteranceを経由することなく）直接Vec<AccentPhraseModel>を生成することを試みています．個人的にはunwrapが減り，全体的な見やすさは向上したと考えていますが，他の方からは違って見えるかもしれません．

関連 Issue

ref #740
ref #731 （jpreprocessのインターフェースがjlabelであるため）

その他

@qryxip qryxip mentioned this pull request Feb 15, 2024
Merged
@qryxip qryxip self-requested a review February 17, 2024 18:45
@phenylshima phenylshima marked this pull request as ready for review February 17, 2024 22:05
qryxip
qryxip reviewed Feb 20, 2024
View reviewed changes
Copy link
Member

@qryxip qryxip left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ほぼLGTMです!

jlabelの責任の付近はあまり精査していませんが、おおむね問題無いと思いました。元の処理を踏まえつつ、簡潔で良い実装になったと思います。

@Hiroshiba 処理自体は順番とかが変わっているので、方針を含めた確認をお願いできますか?

crates/voicevox_core/src/engine/full_context_label.rs Outdated Show resolved Hide resolved
@qryxip qryxip requested a review from Hiroshiba February 21, 2024 15:24
Hiroshiba
Hiroshiba reviewed Feb 21, 2024
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

プルリクエストありがとうございます！！

ここはTTSの処理の根幹の部分で重要度が高い箇所なのですが、ロジックが変わっている可能性があるのがちょっと気になりました！
もちろんコードを見る感じ変わってないようには見えるのですが、往々にして僕たちはよく見逃してしまう気がします。
念のために動作が変わっていないことを保証するため、extract_full_context_labelのテストを書くのはどうでしょう？

  • AccentPhraseが１つだけのもの
  • 読点を含まずAccentPhraseが複数あるもの
  • １文字のAccentPhrase
  • 変わった音素、無声音素・促音（っ）・撥音（ん）
  • ？

あたりがテストケースで網羅されてたら大丈夫かなと思います！
もしよかったらなのですが、今のmainブランチで以下のテキストのAccentPhraseModelsを作成して完全一致するテストを書き、こちらのプルリクエストでも同じテスト回してみるのはどうでしょうか･･･？

テスト用テキスト候補

  • いぇ
  • んーっ
  • これはテストです
  • １、１０００、１００万、１億？

@phenylshima
Copy link
Contributor Author

phenylshima commented Feb 22, 2024 •
edited
Loading

レビューありがとうございます．

提案いただいた例を使って，mainブランチでテストケースを作成しました．
他にも必要と思われるケースがありましたら，遠慮なくおっしゃってください．

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 22, 2024 •
edited
Loading

@femshima 早速ありがとうございます！！お言葉もありがたいです･･･！

ややこしくてすみません！ 🙇
extract_full_context_labelの処理結果が変わっていないことのテストができればと思っていました！
なので例えば今のmainブランチ（VOICEVOX:main）に新しくextract_full_context_labelのテストを実装し、そのテストがmainブランチで通ることを確認した後、今いただいている #742 のプルリクエストを再度テストする、みたいな感じを想像していました！
この流れで頂いているプルリクエストが完全一致することの記録をGithub Actionsのテスト通過として残せる感じです。

でもこの流れはお手数おかけしちゃうので、もしよければで･･･ 🙇

あとこっちはリファクタリングの提案ですが、テストケースでMoraModelインスタンスを作る部分は関数化すればかなりスッキリするかもとか思いました！

@phenylshima phenylshima mentioned this pull request Feb 22, 2024
Merged
@phenylshima
Copy link
Contributor Author

なるほど，そういうことでしたか！
#750 を作成しましたので，見ていただければ幸いです．

@qryxip
Copy link
Member

qryxip commented Feb 23, 2024

ところでなんですが、このPRをマージしたらstruct AudioQueryModelとかは全部AudioQueryとかにできますね。
(jlabel::Moraとの区別については、jlabel::Moraとcrate::Moraとでもしておけばよいはず)

@phenylshima
Copy link
Contributor Author

#750 のマージ，ありがとうございました．
このPRでも重複箇所の削除（mainからのdiffとしてはテストの追加とインターフェースの修正）を行いました．

Hiroshiba
Hiroshiba approved these changes Feb 25, 2024
View reviewed changes
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！！！

お手数おかけしました！
おかげさまで検証記録をしっかり OSS として残せました。とてもよかったと思います！！

@Hiroshiba
Copy link
Member

変更があるので念のために @qryxip さんの確認があってもいいかもと思いました。
@qryxip さん的に問題なければマージしていただければ 🙇

qryxip
qryxip approved these changes Feb 26, 2024
View reviewed changes
Copy link
Member

@qryxip qryxip left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

for文をイテレータからの.collect()にできそうな気はしますが、まあ気が向いたときに別PRで他の箇所のforと一緒にというのでもよいと思います。とりあえずマージしましょう。
(ちなみに私はIterator<Item = Result<Option<_>, _>>を.flat_map(Result::transpose)するやりかたを気に入っています)

@qryxip qryxip merged commit cd17924 into VOICEVOX:main Feb 26, 2024
36 checks passed
@qryxip qryxip mentioned this pull request Feb 26, 2024
Merged
@phenylshima phenylshima deleted the feat/jlabel branch February 26, 2024 21:10
This was referenced Mar 22, 2024
Merged
Closed
@qryxip qryxip mentioned this pull request Jul 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cm-ayf cm-ayf cm-ayf left review comments

@qryxip qryxip qryxip approved these changes

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@phenylshima @Hiroshiba @qryxip @cm-ayf