brianbruggeman / dice

A DSL for rolling dice build in python
MIT License
3 stars 0 forks source link

incorrect prioritization #1

Open M9SCO opened 3 years ago

M9SCO commented 3 years ago

incorrect prioritization for next simple formula: "1d20-3+10"

`DEBUG:dice.parser:pretty: sum dice number 1 number 20

DEBUG:dice.parser:pretty: dice number 1 number 20

DEBUG:dice.parser:pretty: number 1

DEBUG:dice.parser: number => 1 DEBUG:dice.parser:pretty: number 20

DEBUG:dice.parser: number => 20 DEBUG:dice.parser: 1d20 => [19] DEBUG:dice.parser:pretty: add number 3 number 10

DEBUG:dice.parser:pretty: number 3

DEBUG:dice.parser: number => 3 DEBUG:dice.parser:pretty: number 10

DEBUG:dice.parser: number => 10 DEBUG:dice.parser: reduce(add, (3, 10), 0) => 13 DEBUG:dice.parser: reduce(sub, (19, 13), 0) => 6 6 `

his calculate from right to left - not the other way around from left to right like formula 1d20-(3+10) correct result for this example - 26

for next example writing new formula - "1-2+3+4+5+6+7+8" this code reading as his like 1 - (2+(3+(4+(5+(6+(7+8)))))) and send result -26 (correct 32)

brianbruggeman commented 3 years ago

Hey - sorry I didn't see this until today. Thank you so much for this bug report! I haven't looked at the code in a while, though as I recall, this particular parser isn't going to be super great at the math yet and it clearly needs some updates.

When I get a chance, I'll look at this again. And I'll add a test case too. But I'd also accept a pull request if you have an idea what to do for a change.

M9SCO commented 3 years ago

Yes, thanks your repo. I write multiplatform(telegram/discord/) D&D assistant and I wracked my brain when it comes to rolling dices until found u. I learned about Lark and peeked at the notation And I fixed this bug preparing string

from re import sub as re_sub, findall, search

...

for negative in findall("(-\w+)", formula[1:]):
        formula = re_sub(negative, f"+({negative})", formula)

like 4-2 == 4+(-2) right?

and add to notation until number

            | "(" smath ")"

Yes, maybe it's a crap option, but it works and his can get answer like 1d20*(4-(3+2))/2

brianbruggeman commented 3 years ago

I went with the LALR form of Lark when I initially built this library. There is a second option, Earley, which is supposed to provide better control with the caveat that it may lose performance. The LALR form of Lark provides no mechanism to support Precedence (or I suppose Lark may call it prioritization). So I'm left with rebuilding a portion of the code using Earley as a test to see whether or not it can actually handle left to right rather than right to left.

Having spent a little bit of time looking into an Earley parser form, from what I can tell, setting the priority field to any of its four options and setting the parser field with parser=Earley (which is the default) does not actually change how the tree is constructed. As a consequence, we still end up with the same problem with this simple formula:

In other words, the Lark lexer yields a tree with the following:

As a result, I'm of the opinion that Lark doesn't actually support what I want to do with it. I did ping the gitter for Lark, but received very little help. That makes me think that it really cannot do what we need it to do for this output.

As you've mentioned, adding parenthesis will give a work-around though the formula will be less beautiful with excess parenthesis. That aside, I really would like to add a parenthesis to the EBNF anyway just for keeping things easy to read with some of the formulas.

Also, you mentioned that an additional possible work-around was to replace a minus sign, -, with an addition and a negative sign: <left-hand> + -<right hand> or as an example 1 - 2 becomes 1 + -2. And it's also possible that we could add that specific construct to the EBNF notation.

A third option is to potentially drop Lark in favor of a PEG parser that can handle order of precedence. At the time, I thought Lark looked new and interesting as a LALR because of its speed.

A final option is swap the underlying code with another language (I'd pick Rust at this point) and have it expose a Python interface that effectively has the same API but actually "works" as desired. I like this option because the performance here would be far better than the current iteration. And, I'd like to really make an example for myself where I can build an extension for Python in Rust. But this will take some time and effort.

For right now, I believe I'm going to take your two suggestions and then add them to the code base and re-release as 0.4. This will be the least effort and provide a work-around soonish.