feat: 自動調整によって擬似的に疑問文に対応する #229
Conversation
Pull Request Test Coverage Report for Build 1597983395
💛 - Coveralls |
Coverage ResultResultを開く
|
|
ラベルのメンテで説明がないと100%積むのでソースコード中のコメントで #222 (comment) に書いてある以下の説明があると良いと思いました
|
|
CIでエラー出てるようなので修正します |
qwerty2501
force-pushed
the
feature/question
branch
from
3dabd01 to
04d4e05
Compare
ジャストアイデアですが、「ん」や「っ」で終わる疑問文でエラーが出ないかテストしておくと良いかもと感じました。 |
|
とりあえず1ケースだけ思いのままに書いて追加してみましたがこんな感じでいいですかね？ |
色んなパターンを作ってみんなで聴き比べられると、納得の行く答えにたどり着けるかも・・・？ |
deep copyのほうがよりコードのimmutabilityを主張するのに適してるようなので変更 VOICEVOX#229 (comment)
|
機能実装に関してコードを眺めていて、課題が２つ思いついたので共有します。
|
これはこのPRでやったほうがよさそうですね
全く新しいところの修正なのでissue分けたほうが良いと思います |
qwerty2501
force-pushed
the
feature/question
branch
2 times, most recently
from
54fce89 to
894bd1e
Compare
Fixed
実装にバグがあったため音声ファイルも上げ直してます |
qwerty2501
force-pushed
the
feature/question
branch
from
eac3f33 to
79a2b97
Compare
run.py
Outdated
| accent_phrases = engine.create_accent_phrases( | ||
| text, speaker_id=speaker, enable_interrogative=enable_interrogative | ||
| ) |
There was a problem hiding this comment.
API、良いと思います！
run.py内にcreate_accent_phrasesがいくつかあって、それらのAPIにもenable_interrogative: bool = Trueをつけると一貫性があるかなと思います！
There was a problem hiding this comment.
とりあえずrun.pyにあるもので create_accent_phrases を呼び出してると思われるところにつけました
良いと思います！！ |
|
#235 に作りました |
test/test_synthesis_engine.py
Outdated
| ) | ||
| self.assertEqual(expected, actual) | ||
|
|
||
| def test_create_accent_phrases_これはありますか(self): |
There was a problem hiding this comment.
日本語関数名になっているのが気になりました。
まあ日本語処理のテストで、外部から使うこともないのでいいかなとも思うのですが、consonant+vowelの形でASCIIにするのはどうでしょう？
There was a problem hiding this comment.
これは迷ったんですが、関数名がテストケース名になるようなのでわかりやすさを重視した感じですね
私個人はどちらでもよかったんですが、テスト観点のデータが日本語でコメント等も日本語で運用されており、ほぼ完全に日本語コミュニティなのでまあケース名も日本語にしておくかぐらいの気持ちでした。
このままにするにしてもコメントで上記理由ぐらいは書いておこうと思います
There was a problem hiding this comment.
テストはそのコードが意図通りに上手く行っているかを検査するのが目的だと思うので、そのコードの意図を正確に伝えられるのであれば日本語を使うのもありかなと感じました。
ただこの場合は、テストの意図（「create_accent_phrasesをテストしたい」という意図）の情報量が増えないので、そもそも例文（「これはありますか」）を含めなくても良い気がしました。
There was a problem hiding this comment.
IMO: それは一つのテスト関数で複数のデータケースを確認してしまうという意味だと思いますが、pytestだと関数名=ケース名となってるようなので、テストでエラーになったときにどのケースでエラーが起きたかわかるようにする必要があると思いました。また、実行確認の際にもpytestだと個別で実行するには関数単位でしか分けられない？ようなのでこのテストケースだけ確認したいというときにちょっと困ったことになりそうです。もし上手くやれる方法があるなら教えていただけますでしょうか？
以上を踏まえて、 test_create_accent_phrases にテスト関数名を変更しますか？
There was a problem hiding this comment.
たしかにログに出ないとわかりにくいかもです。
pytestのfixtureとparametrizeを使うと、入力データをログに出すことができるので、これを用いるのが最高かもです。
https://dev.classmethod.jp/articles/pytest-parametrize-fixture/
（pytestのparametrizeは結構癖があるので、正直なかなかしんどいと思います･･･）
実行が重いテストは個別に実行したくなりそうですが、このテストは軽いのでまとめて実行しても良いかなと感じました。
以上を踏まえて、 test_create_accent_phrases にテスト関数名を変更しますか？
誰も強い意見を持っていない状態だと思うので、一旦どちらでも良いかなと感じました。
確認したところ、他のテストはデータ例をテスト名に含めていなそうので、とりあえず合わせるために消す、というのはどうでしょう。
There was a problem hiding this comment.
私はテストを汎用的にする(特定の関数などに対するテストを全てひとつのテスト関数に集約する)ことを念頭に今までテストの実装をしていたので、どちらかと言うと関数名にこれはありますかを入れるのは消極的です。
合わせるという意味もありますが、テストをより汎用的にするという意味でもtest_create_accent_phrasesでとどめておくと、後々のためにも良いのかなと思いました。
どちらにせよ、テストが落ちた時にはどこで落ちたか(どの行を実行中に失敗したか)を見るので、個人的には具体的なテストケース名を入れなくて良いかなと思っているのもひとつあります。
There was a problem hiding this comment.
@Hiroshiba @y-chan はい。一旦合わせる感じでまとめちゃいますね。
There was a problem hiding this comment.
既存と同じ感じにしてみました。assertEqualでメッセージ出せるようだったのでそこに申し訳程度にどのケースだったかわかるようにメッセージを出すようにしてます。
@Hiroshiba parametrizeを使うことも検討しましたが、expectedであるAccentPhraseの出力がカオスになりそうな気がしたので一旦見送りました。まあこの手のケース取り扱うライブラリはないかしら癖があるので挙動確認してみて使い物になりそうだったら導入を検討してみてもいいかもしれませんね。
voicevox_engine/model.py
Outdated
| is_interrogative: bool = Field( | ||
| title="疑問の発音を表現するモーラかの判定", default=False | ||
| ) # frontが動作しなくなるためdefault値を設定するようにしている # noqa |
There was a problem hiding this comment.
対応の必要があれば、いったんマージしてしまってからでいいと思いますが、一応書いておきます。
Fieldにデフォルト値を設定すると、フロントエンド側でOptional（おそらくtype | undefined）なフィールドになるようです。
Optionalを外すのに、デフォルト値をフロントエンド側のいろんなところに書くことになるとつらいので、 #13 がマージされたのだと思っています。
また、 #43 でAPIの互換性維持の方針として 、
バージョンが上がっても、
/audio_queryで返ってくる値をそのまま/synthesisに POST すれば音声合成できるようにする予定です
ということなので、AudioQuery内部の互換性については、それぞれのケースに任されていると思っています。
（つまり、サードパーティがAudioQueryを保存して、あとで再利用するような機能を持っている場合に、その互換性については必ずしもサポートしなくていい、というつもりでいい）
ここでAudioQueryの互換性を切るか、切らないかという判断がありそうです。
影響するのは、フロントエンド側でis_interrogativeなモーラであることをUIに反映する場合でしょうか。
わたしはフロントエンド側のAPIを使うコードを書いていないので、判断が難しいです。
個人的には、 いったん互換性を重視して、デフォルト値を設定する形でマージしてしまっていいかなと思いました。
互換を切る（デフォルト値を設定しない）場合、新しいPreviewビルドを作る形かなと思いました。
@Hiroshiba どうでしょう?
There was a problem hiding this comment.
デフォルト値ありで良いと思います！
というのも今回のis_interrogativeは、エディタ側からは設定画面やPOST処理だけに使用するので、オプショナルでもそこまで大変じゃなさそうなためです。
There was a problem hiding this comment.
影響するのは、フロントエンド側でis_interrogativeなモーラであることをUIに反映する場合でしょうか。
現状使用する予定はないので、使用する時に対応すればよいと思います。
default値を設定した理由としてはフロント側の負荷軽減が目的です。これを設定することにより、フロント側ではなにも修正しなくてよいはずです。
将来的には is_interrogative が trueだったら エディタ画面のMora表示のところに ? を出すといったことはしても良いかもしれませんね。
There was a problem hiding this comment.
あ、ほんとだ、たしかに現状使用予定が無いですね…
そして将来的に使うかも微妙かもです。
APIは消去より追加のがだいぶ簡単なので、いったん無しにした方が良いかなと感じました！
There was a problem hiding this comment.
APIは消去より追加のがだいぶ簡単なので、いったん無しにした方が良いかなと感じました！
ちょっと気になったのですが、ここのフィールド宣言を消してしまうと内部処理で使えなくなるのではないですか？
Pythonは動的型付け言語だからいけるんですかね。
無しにしたほうが良いとは何をさしてるのかちょっとわかりませんでした
There was a problem hiding this comment.
こんな感じです。
new_accent_phrases = []
for accent_phrase in accent_phrases:
if accent_phrase.is_interrogative:
accent_phrase = adjust_interrogative(accent_phrase)
new_accent_phrases.append(accent_phrase)There was a problem hiding this comment.
そのコードだと、accent_phrase に is_interrogative を持っていることを期待しているようですが、 adjust_interrogative は SynthesizeEngineによるmora dataの変更適用後の情報である必要があるためここの accent_phrase は model.py にある AccentPhrase である必要があると思います。
つまり、この実装を行ってもAPI定義の変更が必要になるかと。
以下のコメントから余計なAPI定義の変更は行わないという文脈だと思ってたので、この実装を行っても根本的な問題解決にはならないのではないかと考えていたんですが違ってましたか？
あ、ほんとだ、たしかに現状使用予定が無いですね…
そして将来的に使うかも微妙かもです。
APIは消去より追加のがだいぶ簡単なので、いったん無しにした方が良いかなと感じました！
There was a problem hiding this comment.
すみません、ちょっと意図をコードに落とし込めていませんでした･･･
is_interrogativeをAccentPhrasemodelに持たせるのではなく、何らかの方法で別でis_interrogativeを持っておくという感じです。
こんな感じになりそうです。
new_accent_phrases = []
for accent_phrase, is_interrogative in zip(accent_phrases, is_interrogative_list):
if is_interrogative:
accent_phrase = adjust_interrogative(accent_phrase)
new_accent_phrases.append(accent_phrase)There was a problem hiding this comment.
それだとSynthesisEngineBaseのコードが複雑化しそうですが、そのへんは許容していただけるということでいいですかね
実際にコード見てみた感じAccentPhraseに疑問系のブール値もたせるならそうでもないかなと感じたので大丈夫そうです
There was a problem hiding this comment.
@Hiroshiba サンプルとは違いbool値のlistではなくfull_context_labelのAccentPhraseのListで実装してみました。特段boolのみする必要がなくこっちのほうが楽だったため
やりたいこととしてはやれてると思うので問題ないと思います。
voicevox_engine/model.py
Outdated
| is_interrogative: bool = Field( | ||
| title="疑問の発音を表現するモーラかの判定", default=False | ||
| ) # frontが動作しなくなるためdefault値を設定するようにしている # noqa |
There was a problem hiding this comment.
無しにしたほうが良いというのは、宣言を消し、代入もしないという意図でした。
ここのフィールド宣言を消してしまうと内部処理で使えなくなるのではないですか？
今adjust_interrogative_moraは全てのmoras（accent phrase？）で呼ばれていて、内部でis_interrogativeを見ている関係で、内部処理に使われているんだと思います。
これを疑問文のところにだけadjust_interrogative_moraを呼ぶようにするというのはどうでしょう。
test/test_synthesis_engine.py
Outdated
| ) | ||
| self.assertEqual(expected, actual) | ||
|
|
||
| def test_create_accent_phrases_これはありますか(self): |
There was a problem hiding this comment.
テストはそのコードが意図通りに上手く行っているかを検査するのが目的だと思うので、そのコードの意図を正確に伝えられるのであれば日本語を使うのもありかなと感じました。
ただこの場合は、テストの意図（「create_accent_phrasesをテストしたい」という意図）の情報量が増えないので、そもそも例文（「これはありますか」）を含めなくても良い気がしました。
| def is_interrogative(self) -> bool: | ||
| """ | ||
| 音素が質問(interrogative)であるかを返す | ||
| Returns | ||
| ------- | ||
| is_interrogative: bool | ||
| 音素が質問(interrogative)であるか(True)否か(False) | ||
| """ | ||
| return self.contexts["f3"] == "1" | ||
|
|
There was a problem hiding this comment.
議論していて気づいたのですが、OpenJTalkは疑問文かどうかをphoneme単位ではなくaccent phrase単位で処理しているようなので、PhonemeクラスではなくAccentPhraseクラスに情報を持たせるのが良いのかなと感じました。
（情報の範囲がわかりやすくなって、処理がきれいになりそうだなと感じています）
There was a problem hiding this comment.
そうですね。実装してて AccentPhrase単位でつけられてそうな節はあったのですが、どうも確証がもてなかったのでphonemeそのままで判定してる感じですね。
決め打ちでAccentPhraseに持たせて良いかもしれませんが、 #229 (comment) をさきに終わらせてからのほうが良さそうです。
There was a problem hiding this comment.
AccentPhraseに is_interrogative をもたせました
There was a problem hiding this comment.
音素にis_interrogative（疑問文である）フラグがあるのは混乱に繋がりそうなので消しちゃっても良いかもです。
AccentPhraseでis_interrogative = moras[-1].vowel.is_interrogative()としているところで直接moras[-1].vowel.contexts["f3"] == "1"しちゃうとか。
|
@y-chan 書き直すこと自体は問題はないのですが、その書き方だと以下の問題点があるのでやめたほうが良いと感じました。
そちらの方針ということであれば直しますがIMOとしてはこんな感じですね |
|
@y-chan さんと @qwerty2501 さんのテストケースですが、ベースを作って変更箇所を代入する方法だと、疑問文にする前と後でどこが差分になっているか（テストの意図）がわかりやすいという利点もある気がしました。
テストケースが変わるのは厄介なのはそのとおりなので、deepcopyしたほうが安心だと思います。
総合的に見ると、文量が減るので @y-chan さんの案が良いのかなと思いました。 |
deepcopyだとテストケース作る人間が忘れずに明示的にする必要があるので、local関数でベースとなるデータを生成するようにしたほうがいいかもしれません def test_create_accent_phrases(self)
def koreha_arimasuka_base_expected():
return [AccentPhrase(...),...]
expected=koreha_arimasuka_base_expected()
expected. # ここで各ケースごとの変更を行う
self.create_accent_phrases_test_base(
text="これはありますか？".
expected=expected,
enable_interrogative=True,
) |
|
@Hiroshiba @y-chan test_create_accent_phrasesをコメントで書いた感じに書き直してみました |
|
個人的に気になったところと、API・modelの構成は問題なさそうに感じました！ |
qwerty2501
force-pushed
the
feature/question
branch
from
26c6f63 to
e3814cb
Compare
消し忘れのため Co-authored-by: aoirint <[email protected]>
このテストケースではSynthesisEngineを使用する必要がないため
enable_interrogative引数が不要だったので削除
|
Moraからis_interrogative削除した影響で、使っていない引数が発生したので消しました |
|
お疲れ様でした。 |
内容
疑問文を疑問文ぽく聞こえるように音素を調整する
関連 Issue
refs #222