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

np.roundを用いているところをnp.ceilに置き換えたい #552

Closed
y-chan opened this issue Dec 29, 2022 · 4 comments · Fixed by #852
Closed

np.roundを用いているところをnp.ceilに置き換えたい #552

y-chan opened this issue Dec 29, 2022 · 4 comments · Fixed by #852
Labels
優先度:低 (運用中止)

Comments

@y-chan
Copy link
Member

y-chan commented Dec 29, 2022

内容

Pythonのround関数は、単に四捨五入をするわけではなく、偶数丸めをしています。これは、他の言語と異なる挙動であり、他の言語でエンジンを実装する際などに、影響を及ぼすことがあります。この挙動は、もちろんnp.roundにも引き継がれています。
なので、大体どの言語でも仕様が同じであるnp.ceil(無条件に切り上げ)もしくはnp.floor(無条件切り捨て)に変える方がよいのではないかと思いました。

もしくは、可能であるなら、他の言語と同じ挙動をするround関数を独自に作るのもいいかもと思いました。

Pros 良くなる点

Python独自の仕様に悩まされることがなくなるかも?

Cons 悪くなる点

今までと生成される音の長さが変わる

実現方法

書き換える

@y-chan y-chan changed the title np.roind np.roundを用いているところをnp.cellに置き換えたい Dec 29, 2022
@Hiroshiba Hiroshiba added 要議論 実行する前に議論が必要そうなもの 優先度:低 (運用中止) and removed 機能向上 labels Dec 29, 2022
@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 29, 2022

調べてみるとなかなか面白い議論だなと思いました!

まず規格ですが、どうやらIEEEでは四捨五入ではなく偶数丸めが規格化されている(?)ようでした。
https://ja.wikipedia.org/wiki/%E7%AB%AF%E6%95%B0%E5%87%A6%E7%90%86#IEEE%E4%B8%B8%E3%82%81

それはさておいてroundをceilに変えるかどうかですが、例えばsumを取る前の処理とかでceilを使うと、sumの結果が大きく変わる可能性に気づきました。
例えば

lengths=round([0.1, 0.1, 0.1])
sum_length = sum(lengths)

みたいな処理があったとき、roundをceilにするとsum_lengthの値が大きく変わりそうです。
意外と「とりあえずceil使えば良い」とするのは難しい気がしました。

Pythonで四捨五入にする方法はまあありそう↓なのですが、numpyで効率的にやる方法は全然出てこないですね・・・・・・・・・。
https://stackoverflow.com/a/33019948

@Hiroshiba Hiroshiba changed the title np.roundを用いているところをnp.cellに置き換えたい np.roundを用いているところをnp.ceilに置き換えたい Dec 29, 2022
@qryxip
Copy link
Member

qryxip commented Dec 30, 2022

Python以外で偶数丸めを表現することは普通にできそうです。CPythonの実装でどうやって偶数丸めにしているのか調べたところ、単にこうやっているだけだったようなので、これをそのままやれば模倣できると思います。
https://github.com/python/cpython/blob/3.11/Objects/floatobject.c#L1089-L1094

実際試してみたところ、x86_64のWindows, macOS, Linux上にてRustで計算した値がnp.roundと一致するらしいことを確かめました。
コード: https://github.com/qryxip/test-rust-roundeven/blob/98a6946a4b7c9763478466ae0e346df2d8cf4306/src/main.rs
Actions: https://github.com/qryxip/test-rust-roundeven/actions/runs/3806760340/jobs/6475847019

あとrust-lang/libmというライブラリのlibm::rintは、偶数丸めの挙動をするみたいです。
(なぜ↓の実装でそうなるか理解しきれていませんが)
https://github.com/rust-lang/libm/blob/0.2.6/src/math/rint.rs#L10-L14

@y-chan
Copy link
Member Author

y-chan commented Dec 30, 2022

なるほど。
色々考えてみて、エディタ上での再生位置追従機能と実際の音声がうまく噛み合ってるのは四捨五入をしているからだろうというところはあって、ceilorfloorを使うのは望ましくないことに気づきました。
また、numpyで効率的に四捨五入するのは、numpyの実装を独自に書き換えるぐらい大胆なことをしないと厳しいかなと思いました。

なので、ここはもうこの挙動のあり方を受け入れて、roundの挙動が単なる四捨五入ではないということを、参照情報や他言語向けの実装向けの情報を含めてコメントに書く程度にとどめておくのがよいのかな...?と思いました。
これであれば、今までのエンジンに対して破壊的変更を加えることなく、他言語と異なる挙動をすることをコードを読む人に伝えられるかなと...!

どうでしょうか....!

@tarepan
Copy link
Contributor

tarepan commented Dec 11, 2023

roundの挙動が単なる四捨五入ではないということを、参照情報や他言語向けの実装向けの情報を含めてコメントに書く

こちらの方針で着手します。

@tarepan tarepan removed the 要議論 実行する前に議論が必要そうなもの label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
優先度:低 (運用中止)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants