#12780 closed enhancement (fixed)
Be more careful about setting the Maxima 'domain'
Reported by: | mjo | Owned by: | burcin |
---|---|---|---|
Priority: | major | Milestone: | sage-5.6 |
Component: | symbolics | Keywords: | |
Cc: | zimmerma | Merged in: | sage-5.6.beta0 |
Authors: | Michael Orlitzky | Reviewers: | Burcin Erocal |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12845 | Stopgaps: |
Description
Ultimately, we should provide the user a nice way to set this. In the meantime, I'd like to clean up a few places where we play fast and loose with it:
simplify_radical()
andsimplify_log()
set the domain to 'real' before the round trip through Maxima and back. This has no effect on any doctest (radcan ignoresdomain
anyway).
- Expression.expand_log() sets the domain to 'real' when it's called, and 'complex' when it returns. We should make a note of the previous value rather than assuming it is 'complex' when the method is called.
Attachments (1)
Change History (22)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
- Status changed from new to needs_review
comment:3 Changed 7 years ago by
- Dependencies set to #12845
I've refreshed the patch without the fixed doctest. That's now #12845.
comment:4 follow-up: ↓ 5 Changed 7 years ago by
I like the concept of allowing the switch. So you're sure that domain
is ignored for radcan
and for whatever is going on with the log simplify? Just asking.
comment:5 in reply to: ↑ 4 ; follow-ups: ↓ 6 ↓ 14 Changed 7 years ago by
Replying to kcrisman:
I like the concept of allowing the switch. So you're sure that
domain
is ignored forradcan
and for whatever is going on with the log simplify? Just asking.
Dr. Fateman recently (March 14) mentioned on the Maxima list that radcan
was written before Maxima's assumptions framework, and that all simplification takes place outside of radcan
.
I think we can allow the switch where it makes sense. I left the domain: real;
call in expand_log()
alone because it only makes sense to call expand_log()
on a real argument.
With simplify_log()
, it's less clear. Right now, if I do,
sage: f = sqrt(x**2) sage: f sqrt(x^2) sage: f.simplify_log() abs(x)
we silently convert the expression to the reals. This isn't a result of the log simplification algorithm; it's a side effect of setting the domain to real (which provides no other tested benefits).
Obviously assuming that you're working over the reals can allow some simplifications, but we should just make that available to the user rather than doing it arbitrarily in some simplification functions but not others.
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 7 years ago by
With
simplify_log()
, it's less clear. Right now, if I do,
> sage: f = sqrt(x**2) > sage: f > sqrt(x^2) > sage: f.simplify_log() > abs(x)
we silently convert the expression to the reals. This isn't a result of the log simplification algorithm; it's a side effect of setting the domain to real (which provides no other tested benefits).
Hmm, yeah, that seems bad. So all the "usual" results of simplify_log
are still obtained if we remove the domain business (as you are implying)? Then this sounds good.
comment:7 in reply to: ↑ 6 Changed 7 years ago by
Replying to kcrisman:
With
simplify_log()
, it's less clear. Right now, if I do,> sage: f = sqrt(x**2) > sage: f > sqrt(x^2) > sage: f.simplify_log() > abs(x)we silently convert the expression to the reals. This isn't a result of the log simplification algorithm; it's a side effect of setting the domain to real (which provides no other tested benefits).
Hmm, yeah, that seems bad. So all the "usual" results of
simplify_log
are still obtained if we remove the domain business (as you are implying)? Then this sounds good.
Yep, and modulo #12845, the same goes for simplify_radical()
.
comment:8 Changed 7 years ago by
Please fill in your real name as Author.
comment:9 Changed 7 years ago by
comment:10 Changed 7 years ago by
- Reviewers set to Burcin Erocal
- Status changed from needs_review to positive_review
Looks good to me.
comment:11 Changed 6 years ago by
- Merged in set to sage-5.6.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:12 Changed 6 years ago by
See #14305. Apparently radcan does not ignore domain after all.
(%i1) radcan(sqrt(x^2)); (%o1) abs(x) (%i2) domain:complex; (%o2) complex (%i3) radcan(sqrt(x^2)); (%o3) x
This is true in the current Maxima (5.29.1) as well as the older 5.26 series in Sage 5.1/5.2.
comment:13 Changed 6 years ago by
- Cc zimmerma added
comment:14 in reply to: ↑ 5 Changed 6 years ago by
Dr. Fateman recently (March 14) mentioned on the Maxima list that
radcan
was written before Maxima's assumptions framework, and that all simplification takes place outside ofradcan
.
I think that what happened is that this is not part of the assumption framework! There is no assuming going on here, as far as Maxima is concerned. See for instance Fateman's answer here.
comment:15 follow-up: ↓ 16 Changed 6 years ago by
if x
is assumed to be real the correct answer for sqrt(x^2)
is abs(x)
.
However if x
is assumed to be complex, the correct answer is either x
or -x
,
more precisely the one with a positive real part (or a nonnegative imaginary part if the real part is zero). Then if x
is non-real the answer abs(x)
is wrong, since this is the norm of x
, and the norm is real. Consider for example x = -3+4*I
, whose norm is 5, but whose square root is 3-4*I
.
Maple 15 gives:
> assume(x,real); > simplify(sqrt(x^2)); | x~ | > assume(x,complex); > simplify(sqrt(x^2)); csgn(x~) x~
If this ticket did change the default domain of symbolic variables from real to complex, this is a MAJOR change.
Paul
comment:16 in reply to: ↑ 15 Changed 6 years ago by
if
x
is assumed to be real the correct answer forsqrt(x^2)
isabs(x)
.
Again, according to symbolics experts (as opposed to functions experts) like Fateman, there is no such thing as abs(x)
, only x
or -x
, and one then chooses a branch - arbitrarily, but consistently. At least, so I understand that argument.
However if
x
is assumed to be complex, the correct answer is eitherx
or-x
,
Which is what is given here, since one doesn't know a priori whether x
has + or - real part, etc. So Maxima picks x
.
more precisely the one with a positive real part (or a nonnegative imaginary part if the real part is zero). Then if
x
is non-real the answerabs(x)
is wrong, since this is the norm ofx
, and the norm is real. Consider for examplex = -3+4*I
, whose norm is 5, but whose square root is3-4*I
.
Yes, that's presumably why domain:complex
does not allow abs(x)
in Maxima either. Maybe we need a csgn
function too, but I don't know whether Ginac supports this, though apparently it does.
If this ticket did change the default domain of symbolic variables from real to complex, this is a MAJOR change.
The default domain has been complex for a long, long time. It was just exposed here that we didn't do that in simplify_radical
- presumably to avoid the very behavior you are noticing at #14305, but we must have forgotten that.
Note also that I am not advocating for a particular resolution here, just trying to summarize the arguments and previous behavior.
comment:17 follow-up: ↓ 18 Changed 6 years ago by
and one then chooses a branch - arbitrarily, but consistently
I believe when one chooses x
for sqrt(x^2)
, one can always find some inconsistency. For example:
sage: e=sqrt(x^2)-sqrt((x+2)^2) sage: e.simplify_radical() -2 sage: e(x=-1) 0
Paul
comment:18 in reply to: ↑ 17 Changed 6 years ago by
Replying to zimmerma:
and one then chooses a branch - arbitrarily, but consistently
I believe when one chooses
x
forsqrt(x^2)
, one can always find some inconsistency. For example:sage: e=sqrt(x^2)-sqrt((x+2)^2) sage: e.simplify_radical() -2 sage: e(x=-1) 0Paul
You are probably right. I would encourage you to take this up with the Maxima developers on their list, because I personally want to know whether it's even worth thinking about this, or whether mjo is really right and we should just can simplify_radical
, or perhaps relegate it to the deepest recesses of Tartarus.
comment:19 Changed 6 years ago by
Prior to this patch, simplify_radical()
was doing two unrelated things:
- Setting the maxima simplification domain to 'real'.
- Calling radcan().
There was one doctest within sage that incorrectly relied on this, #12845. You can see that I fixed it by making the assumption explicit. It looks like the example in #14305 is doing the same thing. If you expect,
sage: sqrt(x^2).simplify_radical() abs(x)
then you're relying on the implicit conversion to domain:real;
which this ticket changed. You have no reason to expect sqrt(x^2) == abs(x)
unless you assume that x
is real, and we don't. The simplification domain in sage has always been 'complex', except where these sneaky functions twiddled it behind your back. Without the assumption that we're dealing with real numbers, sqrt(x^2)
should simply be left alone. As noted above, you can't "simplify" it without screwing something up.
The real problem in #14305 is that without said assumption, radcan()
will do something ridiculous. That's what radcan()
does. If you want correct answers, don't use radcan()
. The radcan()
function does something very specific, and it works as documented. What it doesn't do is "simplification," and it has no business in sage under the name simplify_foo()
. Please help me kill it: #12737.
comment:20 follow-up: ↓ 21 Changed 6 years ago by
Michael,
ok, we will modify our book, taking into account that by default symbolic variables are considered complex.
However I'm not happy with this ticket (#12780). Before we had (say in 5.1):
sage: assume(x,'real') sage: sqrt(x^2).simplify_radical() abs(x)
This was correct. And now (say in 5.8):
sage: assume(x,'real') sage: sqrt(x^2).simplify_radical() x
This is wrong, thus we have a regression with this ticket.
Paul
Note: I didn't call radcan
, but simplify_radical
...
comment:21 in reply to: ↑ 20 Changed 6 years ago by
Replying to zimmerma:
Note: I didn't call
radcan
, butsimplify_radical
...
The only thing that simplify_radical()
does is call Maxima's radcan()
, and radcan
doesn't do simplification. Instead, it (usually) mangles your expression. That's why I'm so vocally opposed to it being called "simplify."
I agree 100% that the current answer is wrong.. nothing with "simplify" in the name should convert sqrt(x^2)
to x
. But the previous behavior was also wrong. You can leave off the assumption that x
is real, and this will still happen:
sage: sqrt(x^2).simplify_radical() abs(x)
It's less wrong, maybe. But still wrong. In fact, the underlying call to radcan()
wasn't doing anything here. The "simplification" is actually due to the silent switch to the reals. To see this, you can set the Maxima domain, and send your expression for a round trip through Maxima and back. This is in a current version of sage:
sage: maxima_lib.eval('domain: real;') 'real' sage: maxima_lib(sqrt(x^2)) abs(x)
Now that we've fixed that bug (in this ticket), the expression sqrt(x^2)
is passed verbatim to radcan()
. Now, it has something to mangle. And it does. It gives you x
back. So ultimately, the previous, more-correct behavior was the result of a lesser bug preventing radcan()
from doing more damage.
Whoops, there was one failing doctest that I missed (I got a timeout somewhere in ptestlong, I think). Anyway, the doctest was wrong:
simplify_radical()
was doing something it shouldn't have, and the correct fix is to assume some variables are real. Then all we need issimplify()
.This patch could potentially help with fixing
simplify_full()
, but I think it has merit on its own: we shouldn't switch between real/complex behind the user's back. Sometime soon I'll propose a way for the user to set the domain.