Opened 8 years ago

Closed 4 years ago

#12257 closed defect (fixed)

doctest that only integer +/- 1 gets special treatment

Reported by: was Owned by: burcin
Priority: minor Milestone: sage-6.8
Component: calculus Keywords: pynac
Cc: kcrisman Merged in:
Authors: Ralf Stephan Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 906736b (Commits) Commit: 906736b399dbf41cdbdb898c725dad2bf7cc5d70
Dependencies: #18088, #18362 Stopgaps:

Description (last modified by rws)

This is just a doctest of a fix already in main Sage.

Original description:

This is really bad

sage: 1.0 * pi
pi
sage: sin(1.0 * pi)
0

This is good:

sage: maxima('sin(%pi*1.0)')
sin(1.0*%pi)
sage: maxima('1.0*%pi')
1.0*%pi

This is likely the fault of ginac doing something stupid with 1.0 == 1.

Ken Ribet pointed this out during the JMM 2012 meeting.

Change History (29)

comment:1 Changed 8 years ago by burcin

  • Keywords pynac added

There are two sides to this. Doing the multiplication and printing the coefficient.

GiNaC takes a shortcut when the coefficient is 1 and skips multiplication. There is a Python callback function (sage.symbolics.pynac.py_get_parent_char) to check the characteristic of the parent. Pynac forces the operation for positive characteristic. If we return a positive integer from that function when the parent is inexact, the multiplication will go through.

If a coefficient is 1, we don't print it. So the printing code in pynac needs to be changed to print inexact coefficients. This is now pynac issue #4.

Even when all these are sorted out, sin(1.0*pi) might still return 0. Since bool(1.0*pi == pi) will probably be True.

comment:2 Changed 8 years ago by kcrisman

  • Cc kcrisman added

comment:3 follow-up: Changed 8 years ago by was

It should be that sin(1.0*pi) returns 0.0. We can fix this somehow. I'm not worried about printing.

comment:4 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 6 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 follow-up: Changed 5 years ago by kcrisman

Here is another idea. In the Ginac source, we have

00464     } else if (seq_size==1 && overall_coeff.is_equal(_ex1)) {
00465         // *(x;1) -> x
00466         return recombine_pair_to_ex(*(seq.begin()));

and that could be somehow changed. And the other issue is here

00319         if (z.is_equal(*_num0_p))  // sin(0)       -> 0
00320             return _ex0;

so again if there were a way to have Pynac return "the right" zero... I think this is kind of philosophy difference is at the root of several little coercion buglets.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 5 years ago by rws

Replying to kcrisman:

Here is another idea. In the Ginac source, we have

00464     } else if (seq_size==1 && overall_coeff.is_equal(_ex1)) {
00465         // *(x;1) -> x
00466         return recombine_pair_to_ex(*(seq.begin()));

This is currently in Pynac: https://github.com/pynac/pynac/blob/master/ginac/mul.cpp#L741

and that could be somehow changed. And the other issue is here

00319         if (z.is_equal(*_num0_p))  // sin(0)       -> 0
00320             return _ex0;

And this is currently https://github.com/pynac/pynac/blob/master/ginac/inifcns_trans.cpp#L430

The Pynac ticket for this on github is https://github.com/pynac/pynac/issues/4

comment:10 in reply to: ↑ 9 Changed 5 years ago by rws

Replying to rws:

00464     } else if (seq_size==1 && overall_coeff.is_equal(_ex1)) {
00465         // *(x;1) -> x
00466         return recombine_pair_to_ex(*(seq.begin()));

This is currently in Pynac:

Now it's at https://github.com/pynac/pynac/blob/master/ginac/mul.cpp#L713

There may be a general problem, not only for constants:

sage: 1.000000*(x+2)
x + 2
sage: 2.000000*(x+2)
2.00000000000000*x + 4.00000000000000

Is this intended?

Version 0, edited 5 years ago by rws (next)

comment:11 Changed 5 years ago by rws

I think this can be generalized to -1,0,1:

sage: 0.0+5
5.00000000000000
sage: 0.0+x
x
sage: 0.0*x
0
sage: -1.0*x
-x
sage: -2.0*x
-2.00000000000000*x

sage: x^0.0
1
sage: x^1.0
x
sage: x^2.0
x^2.00000000000000
sage: x^-1.0
1/x
sage: x^-2.0
x^(-2.00000000000000)

comment:12 Changed 4 years ago by rws

  • Branch set to u/rws/1_0_pi_should__not__be_pi

comment:13 Changed 4 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to e0e5e9e60caf333d533d2cb94e4f4f5cc21419cb
  • Dependencies set to #18088, pynac-0.3.6
  • Milestone changed from sage-6.4 to sage-pending

Conflicts with #18088, therefore a dependency.


New commits:

4df15dd18088: doctest for fix of pynac issue 20
a853c03Merge branch 'u/rws/inconsistency_with_0_0' of trac.sagemath.org:sage into pynac036
e0e5e9e12257: doctests

comment:14 Changed 4 years ago by rws

  • Branch changed from u/rws/1_0_pi_should__not__be_pi to u/rws/12257

comment:15 Changed 4 years ago by rws

  • Commit changed from e0e5e9e60caf333d533d2cb94e4f4f5cc21419cb to 31afae4c1e4e823322795150c65dfce27df18828
  • Report Upstream changed from N/A to Fixed upstream, but not in a stable release.
  • Status changed from new to needs_review

New commits:

31afae412257: only integer +/- 1 gets special treatment in Pynac

comment:16 Changed 4 years ago by rws

  • Dependencies changed from #18088, pynac-0.3.6 to #18088, #18362

comment:17 Changed 4 years ago by rws

  • Milestone changed from sage-pending to sage-6.7
  • Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

comment:18 Changed 4 years ago by rws

  • Report Upstream changed from Fixed upstream, in a later stable release. to N/A

comment:19 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by kcrisman

It should be that sin(1.0*pi) returns 0.0. We can fix this somehow. I'm not worried about printing.

Even without this patch, we now get (thanks to Pynac upgrade

sage: sin(1.0*pi)
sin(1.00000000000000*pi)

which is perhaps also not ideal, if we wanted 0.00000000000 as in the original intent (I guess) of this ticket. So while I agree that one bug is fixed, perhaps this could be repurposed for that - unless we want to say that sin(1.0*pi) is not exact so we don't actually know it's 0.0, but I feel we don't do that with other things. For instance, even after the Pynac upgrade in #18362, we have

sage: 0.0*pi
0

which is really what is at issue here, I guess. Or?

Last edited 4 years ago by kcrisman (previous) (diff)

comment:20 in reply to: ↑ 19 ; follow-up: Changed 4 years ago by rws

Replying to kcrisman:

sage: sin(1.0*pi)
sin(1.00000000000000*pi)

which is perhaps also not ideal, if we wanted 0.00000000000 as in the original intent

I don't think you can expect 0.00000000000 but rather 1.22464679914735e-16 which is what you get with sin(1.0*pi).n(). I think it's Sage convention (?) that any FP value in an expression should have the effect of automatically applying N() to it.

So while I agree that one bug is fixed, perhaps this could be repurposed for that - unless we want to say that sin(1.0*pi) is not exact so we don't actually know it's 0.0

... even after the Pynac upgrade in #18362, we have

sage: 0.0*pi
0

which is really what is at issue here, I guess. Or?

Yes.

The original ticket was about 1.0. I added to this cases with -1.0 and 0.0 but when fixing stopped short of fixing the 0.0 cases because they seemed quite different from +/-1 and I first wanted to fix where I had a good overview without risk of introducing bugs. So, I think it better if we open a ticket for 0.0 because this case may need a separate discussion.

comment:21 Changed 4 years ago by rws

  • Status changed from needs_review to needs_work

comment:22 in reply to: ↑ 20 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

Replying to rws:

Replying to kcrisman:

sage: sin(1.0*pi)
sin(1.00000000000000*pi)

which is perhaps also not ideal, if we wanted 0.00000000000 as in the original intent

I don't think you can expect 0.00000000000 but rather 1.22464679914735e-16 which is what you get with sin(1.0*pi).n(). I think it's Sage convention (?) that any FP value in an expression should have the effect of automatically applying N() to it.

No, I thought wrong, because I cannot find any function that expands an argument containing a FP number and a constant automatically (contrary to FP only). And I really agree it's wrong because the constant's precision is immediately limited as soon as the FP comes into it. But this is clearly another ticket and the introduced behaviour of sin(1.0*pi) with this ticket is exactly in line with all other functions and arguments at the moment.

comment:23 Changed 4 years ago by git

  • Commit changed from 31afae4c1e4e823322795150c65dfce27df18828 to 447ab202f1ed5ab34ec87c7f73d588acd5c9577a

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

f137553Merge branch 'develop' into t/12257/12257
447ab2012257: fix doctest

comment:24 Changed 4 years ago by rws

  • Milestone changed from sage-6.7 to sage-6.8

comment:25 Changed 4 years ago by rws

  • Description modified (diff)
  • Summary changed from 1.0*pi should *not* be pi to doctest that only integer +/- 1 gets special treatment

The sin(1.0*pi) issue is now #18697.

comment:26 Changed 4 years ago by jdemeyer

I think you need to be slightly more clear what you mean

only integer +/- 1 gets special treatment

I think something like the following would be more clear:

floating point numbers +/- 1.0 are treated differently from integers +/- 1

comment:27 Changed 4 years ago by git

  • Commit changed from 447ab202f1ed5ab34ec87c7f73d588acd5c9577a to 906736b399dbf41cdbdb898c725dad2bf7cc5d70

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

906736b12257: cosmetics

comment:28 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:29 Changed 4 years ago by vbraun

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