Skip to content

create_kanaを疑問文に対応してaudio_query時に?が母音に書き換わってしまうのを抑止するようにした#255

Closed
qwerty2501 wants to merge 2 commits intoVOICEVOX:masterfrom
qwerty2501:feature/improve_create_kana
Closed

create_kanaを疑問文に対応してaudio_query時に?が母音に書き換わってしまうのを抑止するようにした#255
qwerty2501 wants to merge 2 commits intoVOICEVOX:masterfrom
qwerty2501:feature/improve_create_kana

Conversation

@qwerty2501
Copy link
Contributor

内容

かなり場当たり的な修正になった
accent_phrasesだけでは疑問文かどうか判定できないので元になったtextとenable_interrogativeを受け取らなければならずこのような形に・・・

関連 Issue

refs #253

@coveralls
Copy link

coveralls commented Dec 29, 2021

Pull Request Test Coverage Report for Build 1646114330

  • 11 of 11 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 87.375%

Totals Coverage Status
Change from base Build 1645757998: 0.03%
Covered Lines: 789
Relevant Lines: 903

💛 - Coveralls

@github-actions
Copy link

github-actions bot commented Dec 29, 2021

Coverage Result

Resultを開く
Name Stmts Miss Cover
voicevox_engine/init.py 0 0 coverage-100%
voicevox_engine/acoustic_feature_extractor.py 85 0 coverage-100%
voicevox_engine/dev/synthesis_engine/init.py 2 0 coverage-100%
voicevox_engine/dev/synthesis_engine/mock.py 30 1 coverage-97%
voicevox_engine/full_context_label.py 167 5 coverage-97%
voicevox_engine/kana_parser.py 89 1 coverage-99%
voicevox_engine/model.py 65 6 coverage-91%
voicevox_engine/mora_list.py 4 0 coverage-100%
voicevox_engine/preset/Preset.py 12 0 coverage-100%
voicevox_engine/preset/PresetLoader.py 34 1 coverage-97%
voicevox_engine/preset/init.py 3 0 coverage-100%
voicevox_engine/synthesis_engine/init.py 5 0 coverage-100%
voicevox_engine/synthesis_engine/forwarder.py 76 66 coverage-13%
voicevox_engine/synthesis_engine/make_synthesis_engine.py 23 18 coverage-22%
voicevox_engine/synthesis_engine/synthesis_engine.py 108 0 coverage-100%
voicevox_engine/synthesis_engine/synthesis_engine_base.py 54 6 coverage-89%
voicevox_engine/utility/init.py 2 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 35 3 coverage-91%
voicevox_engine/webapi/init.py 0 0 coverage-100%
voicevox_engine/webapi/fastapi_model.py 109 7 coverage-94%
TOTAL 903 114 coverage-87%

@qwerty2501 qwerty2501 changed the title create_kanaを疑問文に対応した create_kanaを疑問文に対応してaudio_query時に?が母音に書き換わってしまうのを抑止するようにした Dec 29, 2021
@qwerty2501 qwerty2501 marked this pull request as draft December 29, 2021 16:07

def create_kana(accent_phrases: List[AccentPhrase]) -> str:
def create_kana(
accent_phrases: List[AccentPhrase], base_text: str, enable_interrogative: bool
Copy link
Member

Choose a reason for hiding this comment

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

たしかにここは疑問文判定をしたいだけなはずなので、base_textを渡すのは余分なデータが多いような印象を受けました。
疑問判定をAPIかこっちのどちらに持たせるかは、おそらくどっちも本当はイマイチで、エンジン内部処理用AccentPhraseを作成して変換するのが最も良いんじゃないかと感じました。
以前仰られていたレイヤードアーキテクチャにおける、データ変換?が今こそ使い時なのかなと思いました。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

とりあえず draftを作りました #256

Copy link
Contributor Author

@qwerty2501 qwerty2501 Jan 5, 2022

Choose a reason for hiding this comment

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

data model分離がrevertされましたのでこの問題をどうするのか決めなくてはなりません。
どうしましょうかね

Copy link
Member

@Hiroshiba Hiroshiba Jan 5, 2022

Choose a reason for hiding this comment

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

やっぱりconvertは便利だと思うので、data model分離でエラーになってしまった部分を直すのがまあ筋かなと感じました。
(時間取らせちゃって申し訳ないです・・・)

直し方ですが、Model.to_engineが無理なので、staticメソッドしかないConverterクラスを作ってConverter.model_to_engineを作るか、以前のようにglobal関数としてconvert_model_to_engineを作るなどの方法を思いつきました。
名前は議論の余地ありだと感じています。
個人的にはConverterにまとめたほうがimportする手間が1回になるので良いのかなと思いました!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

そのあたりこの前converter関数実装する時に調べたのですが、pythonではこういったstatic関数のみの場合はclassに持たせるよりもmodule内にglobal関数として定義したほうがよいと書いてありました。
書き口としてはmoduleに定義した場合でもimportを関数ではなくmoduleにすればほぼ同じになるかと思います。
どっちにしますか?

Copy link
Member

Choose a reason for hiding this comment

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

たしかに・・・

moduleにすると例えば設定値を持たせづらいかもなのでシングルトンクラスとかのが良いかもですが、将来設定値ができたときに変更で良い気がしました。
どういう書き心地になるかちょっと知りたいというのもあるので、今回はmoduleに定義で書いてみる感じを試してみる感じで・・・!

@qwerty2501 qwerty2501 force-pushed the feature/improve_create_kana branch from dba1b13 to 40c2312 Compare January 2, 2022 12:30
@qwerty2501 qwerty2501 marked this pull request as ready for review January 2, 2022 12:32
@qwerty2501
Copy link
Contributor Author

Fixed

data model分離できたので修正しました。
create_kanaに関わる部分のみを修正しようとしたのですが、AccentPhraseにis_interrogativeを持たせてしまうと随所でエラーが出るため結局関わるところ全てを修正せざるを得ませんでした

@qwerty2501 qwerty2501 marked this pull request as draft January 5, 2022 00:13
@qwerty2501
Copy link
Contributor Author

#264 でdata model分離がrevertされたのでdraftにします

@qwerty2501
Copy link
Contributor Author

#272 を修正すると is_kanaをtrueにした状態で /accent_phrases が上手く動かなくなるので、 #273 でここの修正も一緒にやる

qwerty2501 added a commit to qwerty2501/voicevox_engine that referenced this pull request Jan 6, 2022
/accent_phrases時にis_kanaがtrueだとInternal Server
errorが発生するようになったため、 VOICEVOX#255 での修正をこっちに統合した

refs VOICEVOX#253,VOICEVOX#255
@aoirint aoirint closed this in #273 Jan 10, 2022
aoirint pushed a commit that referenced this pull request Jan 10, 2022
* 疑問文の仕様変更反映 #272

- AccentPhraseにis_interrogativeをもたせる
- 疑問符Mora追加を調整前ではなく調整後に行うようにした #272 (comment)

refs #272

* create_kanaを疑問文に対応 #253

/accent_phrases時にis_kanaがtrueだとInternal Server
errorが発生するようになったため、 #255 での修正をこっちに統合した

refs #253,#255

* printが入っていたので消した

* 末尾がptich0でcreate_kanaすると?がつけられてしまうので修正

#273 (comment)
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.

3 participants