Opened 7 years ago

Closed 4 years ago

#12588 closed defect (fixed)

doctest fix for abs(pi*I) should return pi

Reported by: dkrenn Owned by: burcin
Priority: major Milestone: sage-6.8
Component: symbolics Keywords: absolute value, symbolic
Cc: kcrisman Merged in:
Authors: Ralf Stephan Reviewers: Karl-Dieter Crisman, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: c7a2de0 (Commits) Commit: c7a2de021580ca37d57706b2de4ed5e76d11cbd1
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

We have

sage: abs(pi*I)
abs(I*pi)

The result should be simply pi.

Change History (29)

comment:1 Changed 7 years ago by dkrenn

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 7 years ago by dkrenn

  • Description modified (diff)

comment:3 Changed 7 years ago by kcrisman

  • Cc kcrisman added

comment:4 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:6 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:7 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:8 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from abs(pi*i) bug (wrong result) to abs(pi*I) should return pu

comment:9 Changed 5 years ago by jdemeyer

  • Summary changed from abs(pi*I) should return pu to abs(pi*I) should return pi

comment:11 Changed 4 years ago by rws

  • Milestone changed from sage-6.4 to sage-6.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 4 years ago by rws

  • Dependencies set to pynac-0.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 4 years ago by rws

  • Dependencies changed from pynac-0.3.8 to #18537
  • Milestone changed from sage-6.7 to sage-6.8

comment:14 Changed 4 years ago by rws

  • Branch set to u/rws/abs_pi_i__should_return_pi

comment:15 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 8d3356058f645d599b8d43c7acdb09acc63342d6
  • Status changed from new to needs_review

New commits:

4457a3a18537: pynac-0.3.9
8d3356012588: doctests

comment:16 Changed 4 years ago by git

  • Commit changed from 8d3356058f645d599b8d43c7acdb09acc63342d6 to 33e868c92e7cb2627739d717b79ad21854033beb

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

33e868c12588: fix typo

comment:17 follow-up: Changed 4 years ago by kcrisman

  • Reviewers set to Karl-Dieter 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 ; follow-up: Changed 4 years ago by rws

  • 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 4 years ago by dkrenn

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 4 years ago by rws

  • Status changed from needs_review to needs_work

comment:21 Changed 4 years ago by rws

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 4 years ago by git

  • Commit changed from 33e868c92e7cb2627739d717b79ad21854033beb to 25e4324237dfd8fe508c196d809f0957700d463e

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 4 years ago by git

  • Commit changed from 25e4324237dfd8fe508c196d809f0957700d463e to c7a2de021580ca37d57706b2de4ed5e76d11cbd1

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

c7a2de012588: more doctests

comment:24 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

comment:25 Changed 4 years ago by kcrisman

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 4 years ago by rws

After the new development release I'll go through all dependent tickets and adapt/commit the resp. doctests.

comment:27 Changed 4 years ago by rws

  • 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 4 years ago by vdelecroix

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter 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 4 years ago by vbraun

  • Branch changed from u/rws/abs_pi_i__should_return_pi to c7a2de021580ca37d57706b2de4ed5e76d11cbd1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.