pieterderycke / Jace

Jace.NET is a calculation engine for the .NET platform.
MIT License
431 stars 95 forks source link

Bug in operators precedence #79

Open johnnyontheweb opened 1 year ago

johnnyontheweb commented 1 year ago

Hi, I encountered a strange bug. I have this:

A=10
u=2

r1=A*e^(-u^2/2)
r2=A*e^(-(u^2)/2)

and I get

r1=73.8905609893065
r2=1.35335283236613

I'm not an expert, but I think there's an error in operator precedente evaluation for r1 - this should give 1.35 as well as r2.

regards

KarlZ commented 1 year ago

This looks right. For r1 you have -2^2 which is 4 (-2 -2). But in r2 you have -(2^2) or -(2 2) which is -4. Right? So r1 becomes A e^2 while r2 becomes A e^-2

johnnyontheweb commented 1 year ago

No, exponentiation comes first (see https://en.wikipedia.org/wiki/Order_of_operations) than division, the only r2 is right.

KarlZ commented 1 year ago

If r1 were A*e^(0-u^2/2) I would agree. But as written, the "-" sign is not an operator. It is setting the value to be -u, not -(u^2). But, I believe I have the C# code correct here and C# says 73.89 and 1.35 are correct.

[Fact(DisplayName = ("Jace test"))]
public void Test2()
{
    // From: https://github.com/pieterderycke/Jace/issues/79
    var A = 10;
    var u = 2;

    // r1 = A * e ^ (-u ^ 2 / 2)
    var r1 = A * Math.Pow(Math.E, ((Math.Pow(-u, 2) / 2)));
    Console.WriteLine($"r1: {r1}");

    //r2 = A * e ^ (-(u ^ 2) / 2)
    var r2 = A * Math.Pow(Math.E, (-(Math.Pow(u, 2)) / 2));
    Console.WriteLine($"r2: {r2}");
}

results in:

Standard Output: 
r1: 73.8905609893065
r2: 1.353352832366127

Did I write my C# correctly? If not, can you make corrections? I did this in an xUnit test.

johnnyontheweb commented 1 year ago

There's no reason to have (-u)^2, this would always be a positive number.

KarlZ commented 1 year ago

Can you write C# code that behaves like you expect? I would expect Jace to behave exactly like C#.

johnnyontheweb commented 1 year ago

I have it in CalcPad, and it was rendered the same as r2. I was expecting the same from Jace - the point is the minus sign, Jace is an intepreter and I was expecting the same behaviour as others, like CalcPad. Do you believe CalcPad is wrong? In it, you can input:

A = 10
u = 2
r1 = A*e^(-u^2/2)
r2 = A*e^(-(u^2)/2)

and you'll have the same rendering (see pic). calcpad

KarlZ commented 1 year ago

I have not used CalcPad, but I "assume" you typed in that and that it interpreted -u^2 as -(u^2). I'm not sure about CalcPad, but C# would be the gold standard for the way Jace should behave. I am curious how you would get (-u^e) in CalcPad?

johnnyontheweb commented 1 year ago

In CalcPad I type in exacly in the way I showed you. I disagree about the "gold standard", I'd rather prefer that Jace, as an interpreter, treats "-" as an operator and applies precedence (in other words, u is a symbol, not a variable). In fact, CalcPad gives -u^e=-(2^2.72)=-6.58 (exponentiation first)

KarlZ commented 1 year ago

Thanks for reporting the CalcPad issue/question. I didn't even think to double check with Excel. So Excel matches what C# does. But since Jace is a C# library, it really does need to follow the C# (and Excel) rules. I learned something today... I'd never used CalcPad before. Cheers

johnnyontheweb commented 1 year ago

Ok, but in general there aren't language specific operations order, they're common to every language. Today I learned that some behaviours even in basic math can be decided by-design, as it seems CalcPad does (I mean as interpreter, it is written in C# as well). A final thought: I would treat the minus sign as an operator like any other (the minus sign is an operator, since it is not before a number, but a variable), hence I would have written:

    // r1 = A * e ^ (- u ^ 2 / 2)
    var r1 = A * Math.Pow(Math.E, (-(Math.Pow(u, 2) / 2)));
    Console.WriteLine($"r1: {r1}");

but Excel results confused me. Cheers

FabianNitsche commented 1 year ago

@johnnyontheweb is correct. There is already an issue and a PR for this bug: https://github.com/pieterderycke/Jace/issues/62

KarlZ commented 1 year ago

I read the explanation on Operations order? #154 Excellent explanation! Thanks for bringing that up over there @johnnyontheweb What I learned? Use parentheses :-) Unless you want to become an expert in the way every package works (but that is hard for those that must follow in your footsteps)

It doesn't really matter to me (because I reviewed all our code and we don't support such items and do move people down a "use parentheses path") However, as someone that also maintains libraries, this could be a big breaking change for existing users and there is an easy workaround (parentheses). If the library has worked this way all along and something this foundational is changed it could result in many bugs that would be very hard to track down (and not have developers happy when they get to the end).

IF one were to work to make this change, I would do it incrementally. And I would have a way to set this when Jace is started - Precedence.Legacy or Precedence.Wolfram, for example, and start with Legacy as the default? The nature of Jace is that these expressions can be stored in a database. So the Jace user (developer) can't review everything to put in the parentheses. They would need to have a way to keep it in line with the expressions that might already be stored.

johnnyontheweb commented 1 year ago

Maybe using an option for legacy, that by default is false (=no legacy). In my case, the problem is that the same formula should work the same everywhere, hence a correction is needed just to make work fine the existing Jace scripts. In fact, this problem was very hard to find debugging code, since this behaviour was not expected.

KarlZ commented 1 year ago

How do you plan to solve the matter so it works with Excel?

johnnyontheweb commented 1 year ago

IMHO there's no need to mantain the same syntax as Excel. Excel users would have the mentioned option/flag to set. BTW, now I'm checking all my scripts to add parentheses when needed.

vlow commented 12 months ago

Hi all, the PR #63 is merged into our maintained fork of Jace.NET, sonic. Thank you @FabianNitsche for contributing the fix. We had the same issue in production and your PR resolved it.

If anyone is in need of a NuGet package which contains that fix, you're welcome to give sonic a shot.

FabianNitsche commented 12 months ago

@vlow: thanks for the credit and taking over Jace! I will check out sonic.