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 )
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
- Keywords pynac added
comment:2 Changed 8 years ago by
- Cc kcrisman added
comment:3 follow-up: ↓ 19 Changed 8 years ago by
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
- Milestone changed from sage-5.11 to sage-5.12
comment:5 Changed 6 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:6 Changed 6 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:7 Changed 5 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:8 follow-up: ↓ 9 Changed 5 years ago by
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 5 years ago by
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
00464 } else if (seq_size==1 && overall_coeff.is_equal(_ex1)) { 00465 // *(x;1) -> x 00466 return recombine_pair_to_ex(*(seq.begin()));
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?
comment:11 Changed 5 years ago by
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 5 years ago by
- Branch set to u/rws/1_0_pi_should__not__be_pi
comment:13 Changed 5 years ago by
- Commit set to e0e5e9e60caf333d533d2cb94e4f4f5cc21419cb
- Dependencies set to #18088, pynac-0.3.6
- Milestone changed from sage-6.4 to sage-pending
comment:14 Changed 5 years ago by
- Branch changed from u/rws/1_0_pi_should__not__be_pi to u/rws/12257
comment:15 Changed 5 years ago by
- 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
- Dependencies changed from #18088, pynac-0.3.6 to #18088, #18362
comment:17 Changed 5 years ago by
- 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
- 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
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.
comment:20 in reply to: ↑ 19 ; follow-up: ↓ 22 Changed 5 years ago by
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's0.0
... even after the Pynac upgrade in #18362, we have
sage: 0.0*pi 0which 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
- Status changed from needs_review to needs_work
comment:22 in reply to: ↑ 20 Changed 4 years ago by
- 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 intentI don't think you can expect
0.00000000000
but rather1.22464679914735e-16
which is what you get withsin(1.0*pi).n()
. I think it's Sage convention (?) that any FP value in an expression should have the effect of automatically applyingN()
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
- Commit changed from 31afae4c1e4e823322795150c65dfce27df18828 to 447ab202f1ed5ab34ec87c7f73d588acd5c9577a
comment:24 Changed 4 years ago by
- Milestone changed from sage-6.7 to sage-6.8
comment:25 Changed 4 years ago by
- 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
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
- Commit changed from 447ab202f1ed5ab34ec87c7f73d588acd5c9577a to 906736b399dbf41cdbdb898c725dad2bf7cc5d70
Branch pushed to git repo; I updated commit sha1. New commits:
906736b | 12257: cosmetics
|
comment:28 Changed 4 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_review to positive_review
comment:29 Changed 4 years ago by
- Branch changed from u/rws/12257 to 906736b399dbf41cdbdb898c725dad2bf7cc5d70
- Resolution set to fixed
- Status changed from positive_review to closed
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. Sincebool(1.0*pi == pi)
will probably be True.