Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symbolic simplification error #11934

Closed
orlitzky opened this issue Oct 17, 2011 · 18 comments
Closed

Symbolic simplification error #11934

orlitzky opened this issue Oct 17, 2011 · 18 comments

Comments

@orlitzky
Copy link
Contributor

I ran into this today with a real function. Sorry I don't have a shorter test case. The attached file should show a simplification which, as far as I can tell, is invalid.

sage: f = QQ(0.25)*(sqrt(2) - 2)*(x + 1)*x**3 - QQ(3)/QQ(8)*(sqrt(2) - 2)*(x + 1)*x**2 - 
QQ(0.25)*(sqrt(2) - 2)*(2*(3*sqrt(2) - 2)*x**2 - 2*(sqrt(2) - 1)*x + sqrt(-8*(4*sqrt(2) - 
7)*x**4 + 16*(3*sqrt(2) - 5)*x**3 - 8*(2*sqrt(2) - 3)*x - 4*x**2 + 4) - 4*sqrt(2) + 
2)*x**3/(sqrt(2)*x**2 + sqrt(2)*x - 2*sqrt(2)) - 1/24*(x + 1)**3*(x**3 - 3*x + 2) + 
QQ(3)/QQ(8)*(sqrt(2) - 2)*(2*(3*sqrt(2) - 2)*x**2 - 2*(sqrt(2) - 1)*x + sqrt(-8*(4*sqrt(2) 
- 7)*x**4 + 16*(3*sqrt(2) - 5)*x**3 - 8*(2*sqrt(2) - 3)*x - 4*x**2 + 4) - 4*sqrt(2) + 
2)*x**2/(sqrt(2)*x**2 + sqrt(2)*x - 2*sqrt(2)) - QQ(1)/QQ(16)*(x + 1)**2*(2*(sqrt(2) - 
3)*x**3 - (3*sqrt(2) - 8)*x**2 + 2*x + sqrt(2) - 4) + QQ(1)/QQ(8)*(x + 1)*sqrt(2) + 
QQ(1)/QQ(96)*(x**3 - 3*x + 2)*(2*(3*sqrt(2) - 2)*x**2 - 2*(sqrt(2) - 1)*x + sqrt(-8*
(4*sqrt(2) - 7)*x**4 + 16*(3*sqrt(2) - 5)*x**3 - 8*(2*sqrt(2) - 3)*x - 4*x**2 + 4) - 
4*sqrt(2) + 2)**3/(sqrt(2)*x**2 + sqrt(2)*x - 2*sqrt(2))**3 + 1/32*(2*(3*sqrt(2) - 
2)*x**2 - 2*(sqrt(2) - 1)*x + sqrt(-8*(4*sqrt(2) - 7)*x**4 + 16*(3*sqrt(2) - 5)*x**3 - 8*
(2*sqrt(2) - 3)*x - 4*x**2 + 4) - 4*sqrt(2) + 2)**2*(2*(sqrt(2) - 3)*x**3 - (3*sqrt(2) - 
8)*x**2 + 2*x + sqrt(2) - 4)/(sqrt(2)*x**2 + sqrt(2)*x - 2*sqrt(2))**2 - QQ(0.25)*x - 
QQ(1)/QQ(8)*(2*(3*sqrt(2) - 2)*x**2 - 2*(sqrt(2) - 1)*x + sqrt(-8*(4*sqrt(2) - 7)*x**4 + 
16*(3*sqrt(2) - 5)*x**3 - 8*(2*sqrt(2) - 3)*x - 4*x**2 + 4) - 4*sqrt(2) + 
2)*sqrt(2)/(sqrt(2)*x**2 + sqrt(2)*x - 2*sqrt(2)) + QQ(0.25)*(2*(3*sqrt(2) - 2)*x**2 - 2*
(sqrt(2) - 1)*x + sqrt(-8*(4*sqrt(2) - 7)*x**4 + 16*(3*sqrt(2) - 5)*x**3 - 8*(2*sqrt(2) - 
3)*x - 4*x**2 + 4) - 4*sqrt(2) + 2)/(sqrt(2)*x**2 + sqrt(2)*x - 2*sqrt(2)) - QQ(0.25)
sage: f.full_simplify()
-1/24*(sqrt(2)*x^8 - 2*(sqrt(2) - 3)*x^7 - (14*sqrt(2) - 15)*x^6 + 10*(9*sqrt(2) - 
13)*x^5 - (93*sqrt(2) - 128)*x^4 - 4*(9*sqrt(2) - 14)*x^3 + (58*sqrt(2) - 77)*x^2 + 4*
(sqrt(2) - 2)*x - sqrt(2*(4*sqrt(2) - 7)*x^2 + 4*(sqrt(2) - 2)*x - 1)*((16*I*sqrt(2) - 
28*I)*x^4 + (-24*I*sqrt(2) + 40*I)*x^3 + (8*I*sqrt(2) - 12*I)*x + 2*I*x^2 - 2*I) - 
8*sqrt(2) + 10)/(sqrt(2)*x^2 + 4*sqrt(2)*x + 4*sqrt(2))

Component: symbolics

Author: Michael Orlitzky

Reviewer: Jeroen Demeyer

Merged: sage-5.13.rc0

Issue created by migration from https://trac.sagemath.org/ticket/11934

@orlitzky
Copy link
Contributor Author

Attachment: simplify_error.sage.gz

A sage file that displays and attempts to plot the simplification

@kcrisman

This comment has been minimized.

@kcrisman
Copy link
Member

comment:1

Can you be more specific about the invalidity? I think the plot errors are because of the imaginary pieces. Remember, these simplifications are not supposed to be 100 percent valid at all times; especially with roots there are branch issues, unfortunately. The f in question is pretty long - any sense as to where it might simplify in an unusual way?

@kcrisman

This comment has been minimized.

@orlitzky
Copy link
Contributor Author

comment:3

Replying to @kcrisman:

Can you be more specific about the invalidity?

This seems to be the root of the problem. My function is real, the simplification is not:

sage: n(f(x=-0.5))
0.0175781250000000

sage: n(f.full_simplify()(x=-0.5))
0.0175781250000000 - 1.27567374911183e-18*I

I realize that the imaginary part is basically zero, but one of the simplifications has overstepped its bounds somewhere in that the expression is verifiably different pre- and post-simplification.

@jdemeyer
Copy link

jdemeyer commented Nov 3, 2011

Milestone sage-4.7.3 deleted

@jdemeyer jdemeyer removed this from the sage-4.8 milestone Nov 3, 2011
@orlitzky
Copy link
Contributor Author

Dependencies: #12322

@orlitzky
Copy link
Contributor Author

comment:5

This really depends on #12737, but for the patch to apply nicely, #12322 should go in first.

@orlitzky
Copy link
Contributor Author

Author: Michael Orlitzky

@orlitzky orlitzky added this to the sage-5.0 milestone Mar 24, 2012
@kcrisman
Copy link
Member

comment:6

I'm questioning whether this really is fixing anything. First, it's still there with simplify_radical.

sage: f = sqrt(-8*(4*sqrt(2) - 7)*x^4 + 16*(3*sqrt(2) - 5)*x^3)
sage: f.subs(x=-1/2).n()
1.47861134210963
sage: f.simplify_radical().subs(x=-1/2).n()
1.47861134210963 - 9.05388323648765e-17*I

Secondly, the problem is really this. Notice I'm not simplifying anything at all here.

sage: h = I*x^(1/2)
sage: h(x=-1/2)
I*sqrt(-1/2)
sage: h(x=-1/2).n()
-0.707106781186548 + 4.32978028117747e-17*I

Needs work/info, but not because of your patch or #12737, but rather because this doesn't really treat the underlying issue. This could just be some floating point thing that is inherently impossible to avoid once one allows complex numbers (and since your original example is complex sometimes, the answers are going to be complex, unfortunately).

@orlitzky
Copy link
Contributor Author

comment:7

Replying to @kcrisman:

Needs work/info, but not because of your patch or #12737, but rather because this doesn't really treat the underlying issue. This could just be some floating point thing that is inherently impossible to avoid once one allows complex numbers (and since your original example is complex sometimes, the answers are going to be complex, unfortunately).

There aren't any floating point issues if you don't call n() on anything. The issue is still there with simplify_radical(), but that's because simplify_radical() is broken by design: it chooses a branch arbitrarily for the square root. This is what radcan() in Maxima is documented to do, but if you re-brand it as a simplification, it's a bug.

Plain simplify() works:

sage: f = sqrt(-8*(4*sqrt(2) - 7)*x^4 + 16*(3*sqrt(2) - 5)*x^3)
sage: f.simplify()
sqrt(-8*(4*sqrt(2) - 7)*x^4 + 16*(3*sqrt(2) - 5)*x^3)

As does simplify_trig():

sage: f.simplify_trig()
sqrt(-8*(4*sqrt(2) - 7)*x^4 + 16*(3*sqrt(2) - 5)*x^3)

And simplify_rational():

sage: f.simplify_rational()
sqrt(-8*(4*sqrt(2) - 7)*x^4 + 16*(3*sqrt(2) - 5)*x^3)

And simplify_log():

sage: f.simplify_log()
sqrt(-8*(4*sqrt(2) - 7)*x^4 + 16*(3*sqrt(2) - 5)*x^3)

It's only simplify_radical() that messes everything up (note I haven't called n() anywhere, so there are no numerical issues):

sage: f.simplify_radical()
2*I*sqrt((4*sqrt(2) - 7)*x - 6*sqrt(2) + 10)*sqrt(2)*x^(3/2)

I don't want a random branch of the square root when I ask for a simplification. I want a simplification, but only if possible! Without more information, you can't simplify that expression. The rest of the simplify functions don't do anything, but that's the correct thing to do here.

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@orlitzky
Copy link
Contributor Author

Attachment: sage-trac_11934.patch.gz

Doctest for a subexpression of the one in the ticket

@orlitzky
Copy link
Contributor Author

comment:9

I just uploaded a slightly better doctest now that this is fixed.

@jdemeyer
Copy link

jdemeyer commented Dec 7, 2013

Merged: sage-5.13.rc0

@jdemeyer
Copy link

jdemeyer commented Dec 7, 2013

Changed dependencies from #12322 to none

@jdemeyer
Copy link

jdemeyer commented Dec 7, 2013

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link

jdemeyer commented Dec 7, 2013

comment:10

This doesn't seem to depend on #12322 at all.

@orlitzky
Copy link
Contributor Author

orlitzky commented Dec 7, 2013

comment:11

Replying to @jdemeyer:

This doesn't seem to depend on #12322 at all.

Both tickets add a doctest in exactly the same spot, so I added the dep to avoid fuzzing the patches. But you're right, the changes themselves are independent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants