Opened 11 years ago
Closed 7 years ago
#12588 closed defect (fixed)
doctest fix for abs(pi*I) should return pi
Reported by:  Daniel Krenn  Owned by:  Burcin Erocal 

Priority:  major  Milestone:  sage6.8 
Component:  symbolics  Keywords:  absolute value, symbolic 
Cc:  KarlDieter Crisman  Merged in:  
Authors:  Ralf Stephan  Reviewers:  KarlDieter Crisman, Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  c7a2de0 (Commits, GitHub, GitLab)  Commit:  c7a2de021580ca37d57706b2de4ed5e76d11cbd1 
Dependencies:  Stopgaps: 
Description (last modified by )
We have
sage: abs(pi*I) abs(I*pi)
The result should be simply pi
.
Change History (29)
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
Description:  modified (diff) 

comment:3 Changed 11 years ago by
Cc:  KarlDieter Crisman added 

comment:4 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:5 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:6 Changed 9 years ago by
Milestone:  sage6.2 → sage6.3 

comment:7 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:8 Changed 8 years ago by
Description:  modified (diff) 

Summary:  abs(pi*i) bug (wrong result) → abs(pi*I) should return pu 
comment:9 Changed 8 years ago by
Summary:  abs(pi*I) should return pu → abs(pi*I) should return pi 

comment:10 Changed 8 years ago by
comment:11 Changed 8 years ago by
Milestone:  sage6.4 → sage6.7 

Report Upstream:  N/A → Reported upstream. Developers acknowledge bug. 
This is now https://github.com/pynac/pynac/issues/37
Note also
sage: abs(2*pi) abs(2*pi) sage: abs(2.*pi) abs(2.00000000000000*pi)
which is a different issue (in Pynac/abs_eval
the nonnegative case is already handled but obviously does not work here). This is https://github.com/pynac/pynac/issues/39
comment:12 Changed 8 years ago by
Dependencies:  → pynac0.3.8 

Report Upstream:  Reported upstream. Developers acknowledge bug. → Fixed upstream, in a later stable release. 
Resolved in pynac master:
sage: abs(pi*I) pi sage: abs(pi*I*catalan) catalan*pi sage: abs(pi*catalan*x) catalan*pi*abs(x) sage: abs(pi*I*catalan*x) catalan*pi*abs(I*x) sage: abs(1.0j*pi) 1.00000000000000*pi
comment:13 Changed 8 years ago by
Dependencies:  pynac0.3.8 → #18537 

Milestone:  sage6.7 → sage6.8 
comment:14 Changed 8 years ago by
Branch:  → u/rws/abs_pi_i__should_return_pi 

comment:15 Changed 8 years ago by
Authors:  → Ralf Stephan 

Commit:  → 8d3356058f645d599b8d43c7acdb09acc63342d6 
Status:  new → needs_review 
comment:16 Changed 8 years ago by
Commit:  8d3356058f645d599b8d43c7acdb09acc63342d6 → 33e868c92e7cb2627739d717b79ad21854033beb 

Branch pushed to git repo; I updated commit sha1. New commits:
33e868c  12588: fix typo

comment:17 followup: 18 Changed 8 years ago by
Reviewers:  → KarlDieter Crisman 

Status:  needs_review → needs_info 
I note that this doesn't change
sage: abs(i*x) abs(I*x)
(see this long discussion as to connections to derivatives). As long as that's intentional, fine.
In the original discussion leading to this ticket, we have this example, which is still around.
sage: abs(log(2)*I) abs(I*log(2)) sage: abs(log(2.)*I) 0.693147180559945
Again, just wondering if that is intentional, should be another ticket, belongs here... etc. It seems to relate to whether something is a "constant" or just in SR.
sage: abs(euler_gamma*I) euler_gamma sage: abs(5*I) 5 sage: abs(e^5*I) abs(I*e^5)
I guess this would, in principle, belong to this ticket as well, though I understand not wanting to hold up on #18537 since these are sort of intertwined. I don't have any other objections to this, though.
comment:18 followup: 19 Changed 8 years ago by
Status:  needs_info → needs_review 

Replying to kcrisman:
sage: abs(i*x) abs(I*x)
That's because the product's factors are separated into real constants and the rest. If the rest is constant, take abs
of it, else abs().hold()
. Then multiply together. Here I*x
has symbols so it is held.
sage: abs(log(2)*I) abs(I*log(2))
This is because log(2)
is a function expression.
abs(I*e^5)
Here exp(5)
is just printed as e^5
but is a function expression as well.
I haven't thought about how to efficiently determine if an expression with held function values is constant, but I have opened https://github.com/pynac/pynac/issues/56. However I'd suggest not to hold up this ticket for this.
comment:19 Changed 8 years ago by
Replying to rws:
Replying to kcrisman:
sage: abs(i*x) abs(I*x)That's because the product's factors are separated into real constants and the rest. If the rest is constant, take
abs
of it, elseabs().hold()
. Then multiply together. HereI*x
has symbols so it is held. [...] However I'd suggest not to hold up this ticket for this.
Anyhow, since abs(I)
already in current Sage gives 1
, the expression abs(I*x)
should be simplified to abs(x)
.
comment:20 Changed 8 years ago by
Status:  needs_review → needs_work 

comment:21 Changed 7 years ago by
New code in Pynac master is now able to do:
sage: abs(I*x) abs(x) sage: abs(I*pi) pi sage: abs(I*log(2)) log(2) sage: abs(I*e^5) e^5 sage: abs(log(1/2)) log(1/2) sage: abs(log(1/2)*log(1/3)) log(1/2)*log(1/3) sage: abs(log(1/2)*log(1/3)*log(1/4)) log(1/2)*log(1/3)*log(1/4) sage: abs(log(1/2)*log(1/3)*log(1/4)*i) log(1/2)*log(1/3)*log(1/4) sage: abs(log(1/pi)) log(1/pi) sage: abs(log(x)) abs(log(x)) sage: abs(zeta(I)) abs(zeta(I))
comment:22 Changed 7 years ago by
Commit:  33e868c92e7cb2627739d717b79ad21854033beb → 25e4324237dfd8fe508c196d809f0957700d463e 

Branch pushed to git repo; I updated commit sha1. New commits:
466837c  Merge branch 'develop' into t/18537/upgrade_to_pynac_0_3_9

ac542da  18537: bump to 0.3.9.1; fix doctest

9c092c7  18537: doctest for a GiNaC improvement

7590e94  18257: previous doctest should reset variable domains

25e4324  Merge branch 'u/rws/upgrade_to_pynac_0_3_9' of trac.sagemath.org:sage into t/12588/abs_pi_i__should_return_pi

comment:23 Changed 7 years ago by
Commit:  25e4324237dfd8fe508c196d809f0957700d463e → c7a2de021580ca37d57706b2de4ed5e76d11cbd1 

Branch pushed to git repo; I updated commit sha1. New commits:
c7a2de0  12588: more doctests

comment:24 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:25 Changed 7 years ago by
Since apparently Volker has merged #18537, does this now just need checking that tests pass? (And possibly it doesn't need the new Pynac package stuff?) Thanks for responding about https://groups.google.com/forum/#!topic/sagedevel/CVrS4S3AefM
comment:26 Changed 7 years ago by
After the new development release I'll go through all dependent tickets and adapt/commit the resp. doctests.
comment:27 Changed 7 years ago by
Dependencies:  #18537 

Report Upstream:  Fixed upstream, in a later stable release. → N/A 
Summary:  abs(pi*I) should return pi → doctest fix for abs(pi*I) should return pi 
comment:28 Changed 7 years ago by
Reviewers:  KarlDieter Crisman → KarlDieter Crisman, Vincent Delecroix 

Status:  needs_review → positive_review 
This is just a documentation commit that illustrate all the issues raised in the comments. I set to positive review.
comment:29 Changed 7 years ago by
Branch:  u/rws/abs_pi_i__should_return_pi → c7a2de021580ca37d57706b2de4ed5e76d11cbd1 

Resolution:  → fixed 
Status:  positive_review → closed 
The diskussion in sagedevel says that it is related to #6132, #7160 and #10064. Applying the patches from #7160 and #10064 do not solve the problem described above, so I opened this ticket.