Opened 9 years ago
Closed 6 years ago
#12588 closed defect (fixed)
doctest fix for abs(pi*I) should return pi
Reported by:  dkrenn  Owned by:  burcin 

Priority:  major  Milestone:  sage6.8 
Component:  symbolics  Keywords:  absolute value, symbolic 
Cc:  kcrisman  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 9 years ago by
comment:2 Changed 9 years ago by
 Description modified (diff)
comment:3 Changed 9 years ago by
 Cc kcrisman added
comment:4 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:5 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:6 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:7 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:8 Changed 7 years ago by
 Description modified (diff)
 Summary changed from abs(pi*i) bug (wrong result) to abs(pi*I) should return pu
comment:9 Changed 7 years ago by
 Summary changed from abs(pi*I) should return pu to abs(pi*I) should return pi
comment:10 Changed 7 years ago by
comment:11 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.7
 Report Upstream changed from N/A to 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 6 years ago by
 Dependencies set to pynac0.3.8
 Report Upstream changed from Reported upstream. Developers acknowledge bug. to 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 6 years ago by
 Dependencies changed from pynac0.3.8 to #18537
 Milestone changed from sage6.7 to sage6.8
comment:14 Changed 6 years ago by
 Branch set to u/rws/abs_pi_i__should_return_pi
comment:15 Changed 6 years ago by
 Commit set to 8d3356058f645d599b8d43c7acdb09acc63342d6
 Status changed from new to needs_review
comment:16 Changed 6 years ago by
 Commit changed from 8d3356058f645d599b8d43c7acdb09acc63342d6 to 33e868c92e7cb2627739d717b79ad21854033beb
Branch pushed to git repo; I updated commit sha1. New commits:
33e868c  12588: fix typo

comment:17 followup: ↓ 18 Changed 6 years ago by
 Reviewers set to KarlDieter Crisman
 Status changed from needs_review to 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 in reply to: ↑ 17 ; followup: ↓ 19 Changed 6 years ago by
 Status changed from needs_info to 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 in reply to: ↑ 18 Changed 6 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 6 years ago by
 Status changed from needs_review to needs_work
comment:21 Changed 6 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 6 years ago by
 Commit changed from 33e868c92e7cb2627739d717b79ad21854033beb to 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 6 years ago by
 Commit changed from 25e4324237dfd8fe508c196d809f0957700d463e to c7a2de021580ca37d57706b2de4ed5e76d11cbd1
Branch pushed to git repo; I updated commit sha1. New commits:
c7a2de0  12588: more doctests

comment:24 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:25 Changed 6 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 6 years ago by
After the new development release I'll go through all dependent tickets and adapt/commit the resp. doctests.
comment:27 Changed 6 years ago by
 Dependencies #18537 deleted
 Report Upstream changed from Fixed upstream, in a later stable release. to N/A
 Summary changed from abs(pi*I) should return pi to doctest fix for abs(pi*I) should return pi
comment:28 Changed 6 years ago by
 Reviewers changed from KarlDieter Crisman to KarlDieter Crisman, Vincent Delecroix
 Status changed from needs_review to 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 6 years ago by
 Branch changed from u/rws/abs_pi_i__should_return_pi to c7a2de021580ca37d57706b2de4ed5e76d11cbd1
 Resolution set to fixed
 Status changed from positive_review to 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.