cortex-js / compute-engine

An engine for symbolic manipulation and numeric evaluation of math formulas expressed with MathJSON
https://cortexjs.io
MIT License
356 stars 42 forks source link

numeric evaluation of division after selective canonicalization #143

Closed dcbradley closed 6 months ago

dcbradley commented 6 months ago

Description

When using selective canonicalization while parsing, N() evaluates to a rational number rather than a floating point for some expressions.

Steps to Reproduce

const ce = new ComputeEngine();
var expr = ce.parse("-\\frac{1}{2}",{canonical: ["InvisibleOperator"]});                                                                                                                  
console.log(expr.N().json);

Actual Behavior

[ 'Rational', -1, 2 ]

Expected Behavior

-0.5

Environment

compute-engine 0.23.1

Debian 11, node-v20.10.0-linux-x64

I also reproduced the behavior in Firefox 123.0 in macOS 13.6.3, so it's not platform dependent.

Additional Comments

Setting canonical to true or false yields the expected behavior.

Stepping through the code with a debugger, I can see that isCanonical is true, even though this expression was only selectively canonicalized. That prevents it from taking the branch that canonicalizes it during the evaluation, which would avoid the problem. Therefore, it tries to evaluate 'Divide'. However, the N() function is undefined for 'Divide', so it calls evalDivide(), which returns a result in rational form rather than floating point.

I wanted to pitch in by submitting a patch for consideration, but I don't know whether it makes sense to define N() for 'Divide' or whether isCanonical should return false, or perhaps something else.

Working around the problem is not hard, but it mystified me for a while, so I thought I should report it. Thanks for the wonderful library!

arnog commented 6 months ago

Uh. That's a good one. isCanonical should be false after applying partial canonicalization.

In canonical form, division doesn't exist, so the N() handler for divide should not be needed. There's no harm in implementing it, but the proper fix would be to make sure isCanonical is not true after partial canonicalization.