Opened 9 years ago

Closed 9 years ago

# round of symbolic expression (precision issue due to RR)

Reported by: Owned by: dkrenn burcin major sage-5.1 symbolics round symbolic precision RR beginner, sd40.5 kcrisman, mjo sage-5.1.beta5 Mike Hansen Burcin Erocal, Karl-Dieter Crisman N/A

We have

```sage: u = sqrt(43203735824841025516773866131535024) #add one digit
sage: floor(u)
207855083711803944
sage: round(u) # ?? smaller than floor(u)! No warning message...
207855083711803936
sage: r = round(u)
sage: ceil(u)
207855083711803945
```

Numbers with one digit less work

```sage: t=sqrt(4320373582484102551677386613153502)
sage: floor(t)
65729548777426599
sage: round(t) #everything as expected, floor ≤ round ≤ ceil
65729548777426600
sage: ceil(t)
65729548777426600
```

The docstring/source code of the symbolic round mentions that behavior

```Definition:     u.round(self)
Source:
def round(self):
"""
Round this expression to the nearest integer.

This method evaluates an expression in ``RR`` first and rounds
the result. This may lead to misleading results.

EXAMPLES::

sage: t = sqrt(Integer('1'*1000)).round(); t
3333333333333333056287287783757109595393...

This is off by a huge margin::

sage: (Integer('1'*1000) - t^2).ndigits()
984
"""
#FIXME: can we do better?
```

This was reported on sage-support by Lorenzo. I created a ticket here, since I didn't find one for that behavior (but maybe I missed it...).

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

This is the same problem as in #9627 and #9953. We should change those tickets to mention `round()` and close this one. Perhaps `floor()` and `ceil()` can be mentioned there as well. The implementation of `floor.__call__()` in `sage/functions/other.py` might help here.

### comment:2 Changed 9 years ago by kcrisman

• Cc kcrisman added

• Cc mjo added

### comment:4 Changed 9 years ago by mhansen

It might be best to do something along the lines of:

```def round(self):
return floor(self+1/2)
```

but there's the possibility that that addition causes problems with inexact arithmetic.

Last edited 9 years ago by mhansen (previous) (diff)

### comment:5 Changed 9 years ago by mhansen

• Authors set to Mike Hansen
• Status changed from new to needs_review

I've put a patch up which should fix these issues.

### comment:6 follow-up: ↓ 7 Changed 9 years ago by burcin

• Reviewers set to Burcin Erocal

Thanks Mike! This looks great.

Just by reading the code, I have one minor problem: Isn't the test `self > 0` too costly if the expression still contains variables? Maybe we should convert to `RR` first and do the comparison there, similar to the way Gonzalo handled this in #9627. This would also raise an error earlier instead of going through maxima for the comparison and waiting for floor or ceil to complain.

### comment:7 in reply to: ↑ 6 Changed 9 years ago by mhansen

Just by reading the code, I have one minor problem: Isn't the test `self > 0` too costly if the expression still contains variables? Maybe we should convert to `RR` first and do the comparison there

Maybe you could try converting to RIF first instead and then do the full test if that interval contains zero. Checking to see if there are variables should also be really quick. I'll put up a patch that does this.

### comment:8 Changed 9 years ago by mhansen

I've posted a new patch which should be better.

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

• Description modified (diff)
• Status changed from needs_review to positive_review

Looks good to me.

Patchbot, apply attachment:trac_12968.patch.

### comment:10 Changed 9 years ago by jdemeyer

• Status changed from positive_review to needs_work

This causes a doctest failure (see also the Patchbot):

```sage -t  -force_lib devel/sage/sage/rings/polynomial/polynomial_rational_flint.pyx
**********************************************************************
File "/padic/scratch/jdemeyer/merger/sage-5.1.beta2/devel/sage-main/sage/rings/polynomial/polynomial_rational_flint.pyx", line 596:
sage: f.reverse(I)
Expected:
Traceback (most recent call last):
...
TypeError: can't convert complex to int; use int(abs(z))
Got:
Traceback (most recent call last):
self.run_one_example(test, example, filename, compileflags)
File "/padic/scratch/jdemeyer/merger/sage-5.1.beta2/local/bin/sagedoctest.py", line 38, in run_one_example
OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
compileflags, 1) in test.globs
File "<doctest __main__.example_17[19]>", line 1, in <module>
f.reverse(I)###line 596:
sage: f.reverse(I)
File "polynomial_rational_flint.pyx", line 608, in sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint.reverse
(sage/rings/polynomial/polynomial_rational_flint.cpp:7922)
len = <unsigned long> n
File "expression.pyx", line 751, in sage.symbolic.expression.Expression.__int__ (sage/symbolic/expression.cpp:4932)
ValueError: cannot convert I to int
**********************************************************************
```

### comment:11 Changed 9 years ago by mhansen

• Keywords sd40.5 added
• Status changed from needs_work to needs_review

I've posted a new patch which fixes this issue. If someone could look over it, it's just a matter of changing the doctest.

### comment:12 Changed 9 years ago by kcrisman

• Reviewers changed from Burcin Erocal to Burcin Erocal, Karl-Dieter Crisman
• Status changed from needs_review to positive_review

Yes, this is clearly the same behavior for the error. This test passes now, as do many other relevant ones.

### comment:13 Changed 9 years ago by dsm

Just before we put this to bed, I noticed a quirk involving negative numbers. We typically truncate toward zero, like Python:

```Python 2.7.2 (v2.7.2:8527427914a2, Jun 11 2011, 15:22:34)
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
>>> from fractions import Fraction
>>> int(-1)
-1
>>> int(-0.99)
0
>>> int(Fraction(-99, 100))
0
```

But our int-ing is a little more sensitive:

```sage: int(-1)
-1
sage: int(-0.99)
0
sage: int(SR(-99/100))
0
sage: -0.99 == -99/100
True
sage: int(-99/100) # ??
-1

```

### comment:14 Changed 9 years ago by mhansen

This is due to the default rounding mode for rationals, which is independent of this code. We could make a ticket for changing that,

### comment:15 Changed 9 years ago by dsm

Yeah, since the behaviour precedes this patch and this patch makes things better, I left it as positive review. If it were a one-line change (i.e. this behaviour wasn't possibly deliberate) we could have done it here, but instead I'll open a new ticket.

Last edited 9 years ago by dsm (previous) (diff)

### comment:16 Changed 9 years ago by jdemeyer

• Merged in set to sage-5.1.beta5
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.