jiggzson / nerdamer

a symbolic math expression evaluator for javascript
http://www.nerdamer.com
MIT License
517 stars 82 forks source link

Incorrect solution when solving for y in equation of circle #563

Closed ElectrifyPro closed 3 years ago

ElectrifyPro commented 4 years ago

// incorrect solution with circles
nerdamer('solve(x^2 + y^2 = 4, y)'); // [x,-x]

// works correctly with equations of ellipses
nerdamer('solve(x^2 / 9 + (y - 5)^2 / 16 = 1, y)'); // [8*(5/8+sqrt((-1/36)*x^2+1/4)),8*(-sqrt((-1/36)*x^2+1/4)+5/8)]
jiggzson commented 4 years ago

Got it. Thanks.

markhats commented 4 years ago

I've just bumped into this issue as well. Interestingly

nerdamer('solve(2*x^2 + y^2 = 4, y)');  // [(1/2)*sqrt(-8*x^2+16),(-1/2)*sqrt(-8*x^2+16)]

seems to work OK. So it's just when the coefficient of x^2 is 1. Any ideas of a fix for this one?

Thanks.

markhats commented 4 years ago

I've looked into this a bit more and not sure if anyone can help? Basically when solving this type of expression, it first tries to factorize it:

Line 1285 - Solve.js

var factored = core.Algebra.Factor.factor(eq.clone());

For x^2+y^2=4 this incorrectly gives the following factorization: (-x+y)(x+y) This can also be seen using the demo page:

factor(x^2+y^2-5)  // (-x+y)(x+y)

The factorization seems to fail in the "difference of squares factorization" routine (line 2658 of Algebra.js). It seems to ignore the fact there is a constant and factorizes it as if it were: -x^2+y^2.

I can get around the issue for now by bypassing the factorization in the solve function but not sure of the proper way to fix it??

Any ideas?

Thanks.

ElectrifyPro commented 4 years ago

Interesting. When you 'bypass the factorization in solve' as you said, does it return the correct solutions of the circle equation?

Edit: I just tried it and solve indeed gives the correct solution. I think we need to figure out a way to tell the program that a polynomial like this can't be factored, but I'm still working on how to do that

ElectrifyPro commented 4 years ago

I believe I've spotted the problem. In the diff. squares function, there is no check to make sure the signs of the two terms were actually different, which means that a sum of squares could be factored. Could you check the linked PR and help verify this?

markhats commented 4 years ago

Thanks for looking into this.

Your PR does indeed fix the issue for all circle equations that I have tried.

ElectrifyPro commented 4 years ago

Excellent! Then all that's left is to wait for @jiggzson's input.

jiggzson commented 4 years ago

@ElectrifyPro, thanks for tackling this. I definitely think you're on the right track. Your PR does indeed fix the issue but now it appears that two types of expressions no longer factor. Probably something minor.

Failures:
1) Algebra should factor correctly
  Message:
    Expected '-y^2+x^2' to equal '-(-x+y)*(x+y)'.
  Stack:
    Error: Expected '-y^2+x^2' to equal '-(-x+y)*(x+y)'.
        at <Jasmine>
        at UserContext.<anonymous> (C:\Users\Martin\Downloads\nerdamer-master\nerdamer-master\spec\algebra.spec.js:485:39)
        at <Jasmine>
  Message:
    Expected '-b^2*y^2+a^2*x^2' to equal '(-b*y+a*x)*(a*x+b*y)'.
  Stack:
    Error: Expected '-b^2*y^2+a^2*x^2' to equal '(-b*y+a*x)*(a*x+b*y)'.
        at <Jasmine>
        at UserContext.<anonymous> (C:\Users\Martin\Downloads\nerdamer-master\nerdamer-master\spec\algebra.spec.js:485:39)
        at <Jasmine>

Can you think of any reason those are now failing?

ElectrifyPro commented 4 years ago

Hm, when I tried factoring x^2-y^2 and logged the signs of a and b in the sqdiff function, both returned 1, which of course doesn't pass the signs-not-equal-to-each-other check. The sign of the y symbol should be -1, which seems to be causing the problem here.

// index.js
nerdamer.factor('x^2-y^2'); // '-y^2+x^2'
nerdamer.factor('x^2-16'); // '(-4+x)*(4+x)'

// Algebra.js
console.log(a.sign(), b.sign(), a, b); // logs: 1, 1, x, y
if(a.sign() !== b.sign()) {
    factors.add(_.subtract(a.clone(), b.clone()));
    factors.add(_.add(a, b));
    ...
jiggzson commented 4 years ago

@ElectrifyPro, thanks for tracking this down. I'll look into it.

jiggzson commented 3 years ago

@ElectrifyPro, this should be fixed on the dev branch. Please let me know if that's indeed the case.

markhats commented 3 years ago

@jiggzson Thanks - I've tried the dev branch and it fixes solving the circle equations. It still seems to fail on similar equations such as the following but this may be a separate issue??

solve(4/y^2=x^2+1,y)  // [0]
jiggzson commented 3 years ago

@markhats, thanks for looking into this and thanks @ElectrifyPro for tracking this issue down. It's been a big help.

markhats commented 3 years ago

@jiggzson cool - looks to be all working now. Thanks!