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

bug in simplify_radical #8497

Closed
zimmermann6 opened this issue Mar 11, 2010 · 26 comments
Closed

bug in simplify_radical #8497

zimmermann6 opened this issue Mar 11, 2010 · 26 comments

Comments

@zimmermann6
Copy link

the documentation of simplify_radical says:

sage: x.simplify_radical?
...
       Simplifies this symbolic expression, which can contain logs,
       exponentials, and radicals, by converting it into a form which is
       canonical over a large class of expressions and a given ordering of
       variables

however if indeed it is able to recognize zero:

sage: a=1/(sqrt(5)+sqrt(2))-(sqrt(5)-sqrt(2))/3
sage: a.simplify_radical()
0

it does not return a canonical expression:

sage: a1=1/(sqrt(5)+sqrt(2))
sage: a2=(sqrt(5)-sqrt(2))/3
sage: a1.simplify_radical()
1/(sqrt(2) + sqrt(5))
sage: a2.simplify_radical()
-1/3*sqrt(2) + 1/3*sqrt(5)
sage: (a1-a2).simplify_radical()
0

Apply only attachment: 8497_fix_doc.patch

CC: @kcrisman @burcin @jasongrout @mwhansen

Component: calculus

Keywords: simplify, radical, sqrt

Author: Paul Zimmermann, Jeroen Demeyer

Reviewer: Burcin Erocal

Merged: sage-4.7.2.alpha4

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

@zimmermann6
Copy link
Author

comment:1

Note the original question posed to me was: how to multiply say 1/(1+sqrt(2)+sqrt(3)) by the
conjugate expression?

@mwhansen
Copy link
Contributor

comment:2

This is the full docstring from Maxima:

Simplifies expr, which can contain logs, exponentials, and radicals, by converting it into a form which is canonical over a large class of expressions and a given ordering of variables; that is, all functionally equivalent forms are mapped into a unique form. For a somewhat larger class of expressions, radcan produces a regular form. Two equivalent expressions in this class do not necessarily have the same appearance, but their difference can be simplified by radcan to zero.

    For some expressions radcan is quite time consuming. This is the cost of exploring certain relationships among the components of the expression for simplifications based on factoring and partial-fraction expansions of exponents. 

Perhaps we should include this

@mwhansen mwhansen changed the title bug in simplify_rational bug in simplify_radical Mar 11, 2010
@zimmermann6
Copy link
Author

comment:3

Perhaps we should include this

yes (unless of course upstream finds a way to get a real canonical form).
And maybe adding an example showing the difference when checking for 0.

@kcrisman
Copy link
Member

comment:5

What is really going on here is that simplify_radical uses radcan under the hood, which treats things as symbolic expressions, not functions, and just chooses a branch - consistently, but arbitrarily. At least I think that is what is going on here. See the Maxima list thread starting here.

@zimmermann6
Copy link
Author

comment:6

then should we simply change the documentation, in saying that simplify_radical gives a
canonical expression for zero, but if a and b are two identical expressions, they might not
be "simplified" to the same expression?

Paul

@kcrisman
Copy link
Member

comment:7

You are correct. I was just updating, though, at this point.

It gets worse, because some expressions that are definitely not 1 will simplify to just 1, because that is the branch that was picked. See this ask.sagemath.org question, and Fateman's accurate response.

@zimmermann6
Copy link
Author

comment:8

then I suggest to simply remove this function from Sage, unless there are ideas how to fix it.

Paul

@kcrisman
Copy link
Member

comment:9

Well, in Fateman's eyes (and I would remind that he is an expert, if not THE expert, in this), the only bug is in users who treat these expressions as functions. At least, that's how I interpret it. So updating the documentation may be the better way.

But this shouldn't be a duologue; hopefully some others will have ideas. Cc:ing a few others who have thought about at least one or two of these things, just in case they have thoughts at this time. Otherwise it will sit - I simply don't have time to deal with it right now, because it needs to be part of a general overhaul of simplification if we don't just change documentation.

@kcrisman
Copy link
Member

comment:10

I mean change documentation to give examples in prominent places, both in simplify_radical and simplify_full.

@zimmermann6
Copy link
Author

comment:11

I believe we should at least add such examples to the documentation, to warn the user that in some
cases no canonical form is returned.

Paul

@kcrisman
Copy link
Member

comment:12

Okay. So whoever does this ticket will do that :)

(Incidentally, mentioning that they are canonical, but in Fateman's sense of expressions, not in the way we would think of them as functions.)

@zimmermann6
Copy link
Author

Attachment: trac_8497.patch.gz

@zimmermann6
Copy link
Author

comment:13

the attached patch implements what I suggest in comment [comment:11].

Paul

@burcin
Copy link

burcin commented Oct 7, 2011

Author: Paul Zimmermann

@burcin
Copy link

burcin commented Oct 7, 2011

comment:14

Looks good to me.

@burcin
Copy link

burcin commented Oct 7, 2011

Reviewer: Burcin Erocal

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Oct 7, 2011

comment:15

Fixed some formatting of the documentation, needs review.

@jdemeyer
Copy link

jdemeyer commented Oct 7, 2011

Changed author from Paul Zimmermann to Paul Zimmermann, Jeroen Demeyer

@jdemeyer
Copy link

jdemeyer commented Oct 7, 2011

Fixed doc formatting, apply only this

@kcrisman
Copy link
Member

kcrisman commented Oct 7, 2011

comment:17

Attachment: 8497_fix_doc.patch.gz

I feel that we should at least ask on the Maxima list about whether this is truly "not canonical". My understanding is that Fateman would say it is canonical as an expression, not as a function. If I'm the only one who feels this way, I'll let it slide. But I figure we would want him to give us benefit of the doubt in our areas of expertise.

@zimmermann6
Copy link
Author

comment:18

For me, a "canonical expression" means that two mathematically identical expressions simplify to
the exactly same expression. Thus it is clearly not canonical. Feel free to ask on the Maxima
list, but this should not delay this ticket.

Paul

@jdemeyer
Copy link

jdemeyer commented Oct 8, 2011

comment:19

Replying to @zimmermann6:

this should not delay this ticket.

I agree but somebody needs to review my reformatting of the documentation.

@burcin
Copy link

burcin commented Oct 10, 2011

comment:20

I am not well versed in ReST, but AFAICT, Jeroen's changes make sense.

Maxima documentation on radcan() (below) is rather vague. Based on this text, we shouldn't make bold claims about canonical results in the Sage documentation. I am switching this back to positive review.

Simplifies expr, which can contain logs, exponentials, and radicals, by
converting it into a form which is canonical over a large class of expressions
and a given ordering of variables; that is, all functionally equivalent forms
are mapped into a unique form. For a somewhat larger class of expressions,
radcan produces a regular form. Two equivalent expressions in this class do
not necessarily have the same appearance, but their difference can be
simplified by radcan to zero.

For some expressions radcan is quite time consuming. This is the cost of
exploring certain relationships among the components of the expression for
simplifications based on factoring and partial-fraction expansions of
exponents. 

We can open an enhancement ticket to clarify what

    • a large class of expressions*
  • functionally equivalent
    • regular form*
      mean in the text above, and how the ordering of the variables effect the final result. Ideally, we should have references to a description of the underlying algorithm as well.

@kcrisman
Copy link
Member

comment:21

Okay, that's now #11912.

@jdemeyer
Copy link

Merged: sage-4.7.2.alpha4

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

5 participants