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: sage-6.8
Component: symbolics Keywords: absolute value, symbolic
Cc: Karl-Dieter Crisman Merged in:
Authors: Ralf Stephan Reviewers: Karl-Dieter Crisman, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: c7a2de0 (Commits, GitHub, GitLab) Commit: c7a2de021580ca37d57706b2de4ed5e76d11cbd1
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

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 Daniel Krenn

The diskussion in sage-devel 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.

comment:2 Changed 11 years ago by Daniel Krenn

Description: modified (diff)

comment:3 Changed 11 years ago by Karl-Dieter Crisman

Cc: Karl-Dieter Crisman added

comment:4 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:5 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:6 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:7 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:8 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)
Summary: abs(pi*i) bug (wrong result)abs(pi*I) should return pu

comment:9 Changed 8 years ago by Jeroen Demeyer

Summary: abs(pi*I) should return puabs(pi*I) should return pi

comment:11 Changed 8 years ago by Ralf Stephan

Milestone: sage-6.4sage-6.7
Report Upstream: N/AReported 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 Ralf Stephan

Dependencies: pynac-0.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 Ralf Stephan

Dependencies: pynac-0.3.8#18537
Milestone: sage-6.7sage-6.8

comment:14 Changed 8 years ago by Ralf Stephan

Branch: u/rws/abs_pi_i__should_return_pi

comment:15 Changed 8 years ago by Ralf Stephan

Authors: Ralf Stephan
Commit: 8d3356058f645d599b8d43c7acdb09acc63342d6
Status: newneeds_review

New commits:

4457a3a18537: pynac-0.3.9
8d3356012588: doctests

comment:16 Changed 8 years ago by git

Commit: 8d3356058f645d599b8d43c7acdb09acc63342d633e868c92e7cb2627739d717b79ad21854033beb

Branch pushed to git repo; I updated commit sha1. New commits:

33e868c12588: fix typo

comment:17 Changed 8 years ago by Karl-Dieter Crisman

Reviewers: Karl-Dieter Crisman
Status: needs_reviewneeds_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 ; Changed 8 years ago by Ralf Stephan

Status: needs_infoneeds_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 8 years ago by Daniel Krenn

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, else abs().hold(). Then multiply together. Here I*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 Ralf Stephan

Status: needs_reviewneeds_work

comment:21 Changed 7 years ago by Ralf Stephan

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 git

Commit: 33e868c92e7cb2627739d717b79ad21854033beb25e4324237dfd8fe508c196d809f0957700d463e

Branch pushed to git repo; I updated commit sha1. New commits:

466837cMerge branch 'develop' into t/18537/upgrade_to_pynac_0_3_9
ac542da18537: bump to 0.3.9.1; fix doctest
9c092c718537: doctest for a GiNaC improvement
7590e9418257: previous doctest should reset variable domains
25e4324Merge 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 git

Commit: 25e4324237dfd8fe508c196d809f0957700d463ec7a2de021580ca37d57706b2de4ed5e76d11cbd1

Branch pushed to git repo; I updated commit sha1. New commits:

c7a2de012588: more doctests

comment:24 Changed 7 years ago by Ralf Stephan

Status: needs_workneeds_review

comment:25 Changed 7 years ago by Karl-Dieter Crisman

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/sage-devel/CVrS4S3AefM

comment:26 Changed 7 years ago by Ralf Stephan

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 Ralf Stephan

Dependencies: #18537
Report Upstream: Fixed upstream, in a later stable release.N/A
Summary: abs(pi*I) should return pidoctest fix for abs(pi*I) should return pi

comment:28 Changed 7 years ago by Vincent Delecroix

Reviewers: Karl-Dieter CrismanKarl-Dieter Crisman, Vincent Delecroix
Status: needs_reviewpositive_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 Volker Braun

Branch: u/rws/abs_pi_i__should_return_pic7a2de021580ca37d57706b2de4ed5e76d11cbd1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.