AddDice::Parser #147

ysakasin · 2020-03-30T15:02:18Z

AddDiceのパーサーを作成し、実行をさせる。負の数で割れない問題 #59 等を解決する

変更された仕様

1. 目標値に四則演算を含めることができる

1d6>=1+2 等

2. 割り算の表示

旧：(2D6/2) ＞ 2[1,3] ＞ 2
新：(2D6/2) ＞ 4[1,3]/2 ＞ 2

3. 定数の畳み込み

廃止
~~旧：(2D6+1+3) ＞ 4[1,3]+1+3 ＞ 8~~
~~新：(2D6+4) ＞ 4[1,3]+4 ＞ 8~~

4. 負数の割り算ができる

1d6/-3

5. 定数部がある場合、途中式が省略されない

旧：(1D6+1) ＞ 5
新：(1D6+1) ＞ 4[4]+1 ＞ 5

6. AddDice#rollDice以外のメソッドの廃止

AddDice::Parser

文字列をパースして、構文木を生成する。構文木の各ノードが #eval を持っており、それを実行することで計算結果が得られる。

AddDice::ConstantFolding

~~定数畳み込みを行う。 1D6+1+2 ならば 1D6+3 のようにする。~~

検討したがやらなかったこと

NodeとEvaluetorを別ける

Evaluatorの記述が煩雑になるので、Nodeに直接eval関数を持たせた方がわかりやすい

codecov-io · 2020-04-01T08:45:24Z

Codecov Report

Merging #147 into master will increase coverage by 0.01%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
+ Coverage   86.15%   86.16%   +0.01%     
==========================================
  Files         198      204       +6     
  Lines       21013    22044    +1031     
==========================================
+ Hits        18103    18995     +892     
- Misses       2910     3049     +139

Impacted Files	Coverage Δ
src/diceBot/DiceBot.rb	`93.96% <ø> (-0.05%)`	⬇️
src/dice/add_dice/randomizer.rb	`88.70% <88.70%> (ø)`
src/dice/add_dice/parser.rb	`93.93% <93.93%> (ø)`
src/dice/add_dice/node.rb	`99.14% <99.14%> (ø)`
src/bcdiceCore.rb	`73.63% <100.00%> (+2.75%)`	⬆️
src/dice/AddDice.rb	`100.00% <100.00%> (+9.66%)`	⬆️
src/diceBot/CthulhuTech.rb	`98.48% <100.00%> (-0.11%)`	⬇️
src/diceBot/Satasupe.rb	`89.91% <100.00%> (+0.02%)`	⬆️
src/test/add_dice_parser_test.rb	`100.00% <100.00%> (ø)`
src/utils/normalize.rb	`78.57% <0.00%> (-21.43%)`	⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61ec363...1db5580. Read the comment docs.

ochaochaocha3 · 2020-04-02T23:20:57Z

処理結果は問題なさそうです。

ochaochaocha3 · 2020-04-02T23:26:43Z

新しく追加したファイルについて、AddDiceの子クラスが書かれているファイルを src/dice/add_dice/ 以下に置くのはいかがでしょうか？クラスの階層と位置が対応するので、ファイルを探しやすくなります。また、今後のv3でのディレクトリ構成とも近くなります。

ochaochaocha3 · 2020-04-04T15:55:58Z

Evaluatorの記述が煩雑になるので、Nodeに直接eval関数を持たせた方がわかりやすい

全体を見て、今回はこれで問題ないと思いました。現段階ではNodeがAddDice内で完結しており、状況によってeval処理が変わることがないためです。

refs #147 仕様変更をすべてカバーするように、テストケースを追加する。これまでカバーされていなかった仕様は、以下のとおり。 * 1. 目標値に四則演算を含めることができる（`1d6>=1+2` 等） * 4. 負数の割り算ができる（`1d6/-3` 等） * 5. 定数部がある場合、途中式が省略されない * 旧：`(1D6+1) ＞ 5` * 新：`(1D6+1) ＞ 4[4]+1 ＞ 5` 以上に加えて、ダイス1個、定数部なしの場合に最終結果のみが表示されること（例：`(1D6) ＞ 5`）を確認するテストケースを追加する。

src/dice/add_dice/randomizer.rb

src/dice/add_dice/constant_folding.rb

AddDice::Parser: テストケースを追加する

Co-Authored-By: ocha <ochaochaocha3@gmail.com>

構文解析のデバッグを行いやすくするため

AddDice::Node: ノードのS式を返すメソッドを追加する

減算を含む場合に、項の順番に関係なく定数畳み込みの結果が等しくなることを確認する、テストケースを追加する（現状では失敗する）。

ochaochaocha3 · 2020-04-19T12:21:07Z

定数畳み込み（式の簡約化）について、2週間ほど調査や試実装をしてみましたが、抽象構文木を作る場合には、単純で安定した実装にするのがなかなか難しいと分かりました。そのため、これは一旦保留にする（アイデアとして採用しておき、後に詳細な仕様を決めて実装する）のはいかがでしょうか？

現在、テストケースには乗算のみ簡約化するものがあります（https://github.com/bcdice/BCDice/blob/v2.04.00/src/test/data/dummyBot.txt#L96-L100; (2D6*2*3) ＞ 5[2,3]*6 ＞ ... 等）。定数畳み込みの必要性の根拠として挙げられていましたが、処理としては中途半端である（現時点の実装のように加算等の他の演算は簡約化しない）うえ、簡約化を行わなければ混み入って見にくいほど複雑な式でもありません。そのため、現段階では、一旦このテストケースを式の簡約化を行わないもの（例：(2D6*2*3) ＞ 5[2,3]*2*3 ＞ ...）に変更しておく方が無難ではないかと考えました。

ochaochaocha3 · 2020-04-19T12:50:25Z

式の簡約化をまじめに実装する場合に参考になる教科書が見つかりましたので、参考として紹介します。

Joel S. Cohen: "Computer algebra and symbolic computation: mathematical methods", A K Peters (2002).
https://www.ukma.edu.ua/~yubod/teach/compalgebra/%5BJoel_S._Cohen%5D_Computer_algebra_and_symbolic_comp(BookFi.org).pdf

この教科書の3.2節に、各演算の代数学的性質を利用して式を簡約化するアルゴリズムが載っています。十分に正確ですが、再帰を含むかなり複雑なアルゴリズムとなっています。核の部分の一部を簡易に実装した例をGistに上げたので、確認してみてください。
https://gist.github.com/ochaochaocha3/85475f9e107ee70376852bd72ce9f69c

定数畳み込みの失敗例

…ing_test_case AddDice: 定数畳み込み関連のテストケースを追加する

ysakasin · 2020-04-20T14:57:01Z

1d6/2*3 が間違えるという割と致命的で修正が困難な問題が発見されてしまったので、事前計算は要検討とし、このPRからは削除することにします。

[Failures]
Game type: None
Index: 39
Input:
  1d6/2*3
Expected:
  DiceBot : (1D6/2*3) ＞ 6[6]/2*3 ＞ 9
Result:
  DiceBot : (1D6/6) ＞ 6[6]/6 ＞ 1

…ice_parser

ochaochaocha3 · 2020-04-23T08:21:23Z

src/dice/add_dice/node.rb

+        @times = times.literal
+        @sides = sides.literal
+        @critical = critical.nil? ? nil : critical.literal


DiceRollのみ、オペランドを生の数値で持っていますが、何か特別な理由はあるのでしょうか？
他の演算子のようにオペランドをノードとして持っておくと、eval や s_exp などの処理が同じように書ける（それぞれ eval や s_exp を再帰的に適用する形）ので、自然です。
また、このようにインターフェースを合わせておくことで、構文解析処理が変わって times, sides, critical が Number でなくなっても、書き直さずに対応できるという利点があります。

DiceRollがリテラルだからです。 nDxは一見二項演算のように振る舞えますが、nやxにDiceRollが入れ子になると、ダイス結果を展開した中間式を表現することができなくなります。そのため、二項演算と扱うことはできません。

これらの前提から、nやxは乱数を含まない、もしくは整数の演算のみで事前計算ができるはずなのでノードの生成時に引数を整数に変換して問題ないと思います。かえって、ノード生成時に整数に変換できるという制約をいれることで、DiceRollが入れ子になる状況を避けることができます。

ノード生成時に整数に変換できるという制約をいれる

これは単純にコンストラクタで型チェックを行った方が分かりやすいように思いました：

def initialize(times, sides, critical) unless times.is_a?(Number) raise TypeError, "times must be a Number: #{times.class}" end unless sides.is_a?(Number) raise TypeError, "sides must be a Number: #{sides.class}" end # ... end

n や x を整数と見なせることは文法（と括弧内前処理）から分かるので、変換を先に行っても大丈夫なことには納得です。
個人的には、変換も #literal（Numberの内部表現、private でも問題ない）ではなく #eval（評価の公開インターフェース）で行う方がいいかな、と思います。
その方がNumberへの修正に強いです。
今のところ #eval で整数が返るのが保証されているので、型の面でも問題ないです。

#eval は引数にランダマイザを要求するため、コンストラクタで単純に使うことはできません。なので、引数の型だけチェックしようと思いますが、どうでしょう。

今のところは、文法からダイスロールのオペランドが整数になるのは確定するので、型チェックはそれで代用するのでもいいかな、と思いました。

除算ノードのクラスを作り、BinaryOpを単純化する。除算の処理だけが特別なので、汎用的なBinaryOpでその面倒を見るのは重い。そこで、端数処理方法別に除算ノードのクラスを3つ作り、それらに処理を分担させる。継承関係： BinaryOp（二項演算子） | +-- DivideBase（除算の基底クラス） | |-- DivideWithRoundingUp （除算 -> 切り上げ） |-- DivideWithRoundingOff （除算 -> 四捨五入） +-- DivideWithRoundingDown（除算 -> 切り捨て）

ROUNDING_METHOD_SYMBOL では、どうしても Symbol が入っているのを連想してしまうので。

AddDice::Node: 二項演算子をリファクタリングする

ochaochaocha3

大きな変更でしたが、これで大丈夫だと思います。いろいろと意見を取り入れていただき、ありがとうございました。

ysakasin · 2020-04-27T11:28:37Z

@ochaochaocha3
細部までレビュー、提案ありがとうございました！

refs #147 仕様変更をすべてカバーするように、テストケースを追加する。これまでカバーされていなかった仕様は、以下のとおり。 * 1. 目標値に四則演算を含めることができる（`1d6>=1+2` 等） * 4. 負数の割り算ができる（`1d6/-3` 等） * 5. 定数部がある場合、途中式が省略されない * 旧：`(1D6+1) ＞ 5` * 新：`(1D6+1) ＞ 4[4]+1 ＞ 5` 以上に加えて、ダイス1個、定数部なしの場合に最終結果のみが表示されること（例：`(1D6) ＞ 5`）を確認するテストケースを追加する。

AddDice::Parser

ysakasin force-pushed the add_dice_parser branch from bf22132 to fe4845f Compare April 1, 2020 08:43

ysakasin force-pushed the add_dice_parser branch from bf793fb to 9ca80b5 Compare April 1, 2020 13:59

ysakasin added 2 commits April 2, 2020 21:32

AddDice::Parser

d2f634a

Remove rollDiceAddingUp

e3921d1

ysakasin force-pushed the add_dice_parser branch from 9ca80b5 to e3921d1 Compare April 2, 2020 12:47

ysakasin changed the title ~~[WIP] AddDice::Parser~~ AddDice::Parser Apr 2, 2020

ysakasin requested a review from ochaochaocha3 April 2, 2020 13:17

ysakasin added the refactoring 内部構造の改良 label Apr 2, 2020

Move add_dice files

391b860

ochaochaocha3 mentioned this pull request Apr 4, 2020

AddDice::Parser: テストケースを追加する #153

Merged

ochaochaocha3 assigned ysakasin Apr 4, 2020

ochaochaocha3 reviewed Apr 4, 2020

View reviewed changes

src/dice/add_dice/randomizer.rb Outdated Show resolved Hide resolved

ochaochaocha3 reviewed Apr 4, 2020

View reviewed changes

src/dice/add_dice/randomizer.rb Outdated Show resolved Hide resolved

ochaochaocha3 reviewed Apr 4, 2020

View reviewed changes

src/dice/add_dice/randomizer.rb Show resolved Hide resolved

ochaochaocha3 reviewed Apr 4, 2020

View reviewed changes

src/dice/add_dice/constant_folding.rb Outdated Show resolved Hide resolved

ysakasin and others added 6 commits April 5, 2020 11:57

Merge pull request #153 from bcdice/add_dice_parser-add_tests

22c54f7

AddDice::Parser: テストケースを追加する

init value of #inject

d13475c

Co-Authored-By: ocha <ochaochaocha3@gmail.com>

Merge branch 'master' into add_dice_parser

693851e

AddDice::Node: ノードのS式を返すメソッドを追加する

a7066a2

構文解析のデバッグを行いやすくするため

Merge pull request #156 from bcdice/add_dice_parser-s_exp

b76769b

AddDice::Node: ノードのS式を返すメソッドを追加する

AddDice: 定数畳み込み関連のテストケースを追加する

d897938

減算を含む場合に、項の順番に関係なく定数畳み込みの結果が等しくなることを確認する、テストケースを追加する（現状では失敗する）。

AddDice: 符号反転を含む場合のテストケースを追加する

1536c21

定数畳み込みの失敗例

ysakasin added 2 commits April 20, 2020 00:47

Fix folding negative numbers

147820f

Merge pull request #157 from bcdice/add_dice_parser-add_constant_fold…

1fc1bb4

…ing_test_case AddDice: 定数畳み込み関連のテストケースを追加する

ysakasin added 3 commits April 21, 2020 01:48

Remove ConstantFolding

146da3c

Merge branch 'add_dice_parser' of github.com:bcdice/BCDice into add_d…

ae7a956

…ice_parser

Add test and remove duplicated test

a24e968

ochaochaocha3 reviewed Apr 23, 2020

View reviewed changes

ochaochaocha3 and others added 5 commits April 23, 2020 18:09

AddDice::Parser: コメントを追加する

7d7e66f

AddDice::Node: コメントを追加する

466f7de

AddDice::Node: 端数処理方法の定数名を変更する

dc8bf1d

ROUNDING_METHOD_SYMBOL では、どうしても Symbol が入っているのを連想してしまうので。

Merge pull request #162 from bcdice/add_dice_parser-refactor_binary_op

1db5580

AddDice::Node: 二項演算子をリファクタリングする

ysakasin requested a review from ochaochaocha3 April 26, 2020 04:39

ochaochaocha3 approved these changes Apr 26, 2020

View reviewed changes

ysakasin merged commit cd0055b into master Apr 27, 2020

ysakasin deleted the add_dice_parser branch April 27, 2020 11:28

ochaochaocha3 mentioned this pull request May 12, 2020

加算ロールを負の数で割ることができない #59

Closed

ysakasin added a commit that referenced this pull request Aug 30, 2020

Merge pull request #147 from bcdice/add_dice_parser

494443b

AddDice::Parser

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AddDice::Parser #147

AddDice::Parser #147

ysakasin commented Mar 30, 2020 •

edited

Loading

codecov-io commented Apr 1, 2020 •

edited

Loading

ochaochaocha3 commented Apr 2, 2020

ochaochaocha3 commented Apr 2, 2020

ochaochaocha3 commented Apr 4, 2020

ochaochaocha3 commented Apr 19, 2020

ochaochaocha3 commented Apr 19, 2020

ysakasin commented Apr 20, 2020

ochaochaocha3 Apr 23, 2020

ysakasin Apr 23, 2020

ochaochaocha3 Apr 23, 2020

ysakasin Apr 23, 2020

ochaochaocha3 Apr 26, 2020

ochaochaocha3 left a comment

ysakasin commented Apr 27, 2020

AddDice::Parser #147

AddDice::Parser #147

Conversation

ysakasin commented Mar 30, 2020 • edited Loading

変更された仕様

1. 目標値に四則演算を含めることができる

2. 割り算の表示

3. 定数の畳み込み

4. 負数の割り算ができる

5. 定数部がある場合、途中式が省略されない

6. AddDice#rollDice以外のメソッドの廃止

AddDice::Parser

AddDice::ConstantFolding

検討したがやらなかったこと

NodeとEvaluetorを別ける

codecov-io commented Apr 1, 2020 • edited Loading

Codecov Report

ochaochaocha3 commented Apr 2, 2020

ochaochaocha3 commented Apr 2, 2020

ochaochaocha3 commented Apr 4, 2020

ochaochaocha3 commented Apr 19, 2020

ochaochaocha3 commented Apr 19, 2020

ysakasin commented Apr 20, 2020

ochaochaocha3 Apr 23, 2020

Choose a reason for hiding this comment

ysakasin Apr 23, 2020

Choose a reason for hiding this comment

ochaochaocha3 Apr 23, 2020

Choose a reason for hiding this comment

ysakasin Apr 23, 2020

Choose a reason for hiding this comment

ochaochaocha3 Apr 26, 2020

Choose a reason for hiding this comment

ochaochaocha3 left a comment

Choose a reason for hiding this comment

ysakasin commented Apr 27, 2020

ysakasin commented Mar 30, 2020 •

edited

Loading

codecov-io commented Apr 1, 2020 •

edited

Loading