Opened 9 years ago

Closed 5 years ago

# doctest that only integer +/- 1 gets special treatment

Reported by: Owned by: was burcin minor sage-6.8 calculus pynac kcrisman Ralf Stephan Jeroen Demeyer N/A 906736b (Commits) 906736b399dbf41cdbdb898c725dad2bf7cc5d70 #18088, #18362

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

Original description:

```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.

### comment:1 Changed 9 years ago by burcin

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:3 follow-up: ↓ 19 Changed 9 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 7 years ago by jdemeyer

• Milestone changed from sage-5.11 to sage-5.12

### comment:5 Changed 7 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

### comment:6 Changed 6 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:7 Changed 6 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

### comment:8 follow-up: ↓ 9 Changed 6 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: ↓ 10 Changed 6 years ago by rws

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

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

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?

Last edited 6 years ago by rws (previous) (diff)

### comment:11 Changed 6 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 6 years ago by rws

• Branch set to u/rws/1_0_pi_should__not__be_pi

### comment:13 Changed 6 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:

 ​4df15dd `18088: doctest for fix of pynac issue 20` ​a853c03 `Merge branch 'u/rws/inconsistency_with_0_0' of trac.sagemath.org:sage into pynac036` ​e0e5e9e `12257: doctests`

### comment:14 Changed 5 years ago by rws

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

### comment:15 Changed 5 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:

 ​31afae4 `12257: only integer +/- 1 gets special treatment in Pynac`

### comment:16 Changed 5 years ago by rws

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

### comment:17 Changed 5 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 5 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: ↓ 20 Changed 5 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: sin(0.0*pi)
0
```

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

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

### comment:20 in reply to: ↑ 19 ; follow-up: ↓ 22 Changed 5 years ago by rws

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

• Status changed from needs_review to needs_work

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

• Status changed from needs_work to needs_review

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

• Commit changed from 31afae4c1e4e823322795150c65dfce27df18828 to 447ab202f1ed5ab34ec87c7f73d588acd5c9577a

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

 ​f137553 `Merge branch 'develop' into t/12257/12257` ​447ab20 `12257: fix doctest`

### comment:24 Changed 5 years ago by rws

• Milestone changed from sage-6.7 to sage-6.8

### comment:25 Changed 5 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 5 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 5 years ago by git

• Commit changed from 447ab202f1ed5ab34ec87c7f73d588acd5c9577a to 906736b399dbf41cdbdb898c725dad2bf7cc5d70

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

 ​906736b `12257: cosmetics`

### comment:28 Changed 5 years ago by jdemeyer

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

### comment:29 Changed 5 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.