py3: float.__str__ differences

[embray]

I've mentioned this in a few places before, but there wasn't a ticket for it:

In Python 2, float.__str__ and float.__repr__ are different (with only the latter showing 17 significant digits, while the former is arbitrarily truncated to 12:

>>> str(2.0/3)
'0.666666666667'
>>> repr(2.0/3)
'0.6666666666666666'

Whereas on Python 3 float.__str__ and float.__repr__ are the same (exactly, in all cases):

>>> str(2.0/3)
'0.6666666666666666'
>>> repr(2.0/3)
'0.6666666666666666'

For once StackOverflow provides good background: ​https://stackoverflow.com/a/25899600/982257

In fact the new float.__repr__ has been backported to Python 2, but it keeps the old float.__str__.

A result of this is that in Python 3 any tests where float.__str__ is used gives different results, and we need to decide how best to handle that difference.

My personal preference would be to wherever possible in Sage (e.g. in RealDoubleElement and the like) to always use the repr() where it currently uses str(). The ​new repr algorithm is quite a bit nicer.

This would obviously be a backwards-incompatible change in some ways, but would there be much harm?

[jdemeyer]

Replying to embray:

My personal preference would be to wherever possible in Sage (e.g. in RealDoubleElement and the like) to always use the repr() where it currently uses str().

Fine for me.

This would obviously be a backwards-incompatible change in some ways, but would there be much harm?

It's unlikely to break code, but it might break some doctests (but probably not too much since doctests typically use repr() and that will not change).

[tscrim]

My thoughts are the same as Jeroen's.

[embray]

Just by a quick scan of the current test failures on Python 3 it will actually break a lot of doctests in Sage, but that's the point :)

It could also break any doctests in any third-party packages that relying on the str() representation of floats anywhere. It should certainly be documented as a backwards-incompatible change, albeit one that should cause only minor disruption if any.

[jdemeyer]

Replying to embray:

It should certainly be documented as a backwards-incompatible change

Since when do we document changes :-)

[embray]

Oh, right...

[embray]

I think this will actually cover most cases. It seems there were fewer impacted doctests than I thought, I guess maybe since MPFR is used more pervasively than RealDoubleElement.

I'm pretty sure there are some other doctests (e.g. that explicitly call str() on some float value) that might need updating, but not many. I'll check the Python 3 test results and see if there are more I'm missing.

---

New commits:

​5b7da86

Fix RealDoubleElement to use float.__repr__ always, instead of float.__str__, and fix all doctests impacted by that change

[chapoton]

Is this needing rewiew ??

[chapoton]

green bot..

[jdemeyer]

Wouldn't it be better to just remove double_str completely?

[jdemeyer]

And you can probably remove __str__ too.

[embray]

I guess I kept it for backwards compatibility since it is used in at least one other module (though that usage should also be replaced with double_repr).

How strong of an "API" guarantee do we make with cdef functions exported via a .pxd?

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​2d2c844	Fix RealDoubleElement to use float.__repr__ always, instead of float.__str__, and fix all doctests impacted by that change
​5420f79	go ahead and remove double_str entirely, and clean up some now redundant __str__ methods

[embray]

Rebased and removed double_str and a couple __str__.

[chapoton]

Looks good. Jeroen, do you agree ?

[jdemeyer]

I haven't really looked closely, so my opinion is neutral. If you think it's good, you can set this to positive review.

[chapoton]

Then I will give a positive review.

[vbraun]

On 32-bit:

File "src/sage/plot/plot3d/base.pyx", line 1740, in sage.plot.plot3d.base.Graphics3d.stl_ascii_string
Failed example:
    astl.splitlines()[:7]
Expected:
    ['solid surface',
    'facet normal 0.9733285267845754 -0.16222142113076257 -0.16222142113076257',
    '    outer loop',
    '        vertex 2.94871794872 -0.384615384615 -0.39358974359',
    '        vertex 2.95021367521 -0.384615384615 -0.384615384615',
    '        vertex 2.94871794872 -0.39358974359 -0.384615384615',
    '    endloop']
Got:
    ['solid surface',
     'facet normal 0.9733285267845758 -0.16222142113076066 -0.16222142113076066',
     '    outer loop',
     '        vertex 2.94871794872 -0.384615384615 -0.39358974359',
     '        vertex 2.95021367521 -0.384615384615 -0.384615384615',
     '        vertex 2.94871794872 -0.39358974359 -0.384615384615',
     '    endloop']
**********************************************************************
1 item had failures:
   1 of  10 in sage.plot.plot3d.base.Graphics3d.stl_ascii_string
    [337 tests, 1 failure, 46.41 s]
----------------------------------------------------------------------
sage -t --long src/sage/plot/plot3d/base.pyx  # 1 doctest failed

[embray]

Any suggestions for fixing that? Would an # abstol flag be sufficient? I have no way of testing this.

[git]

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

​89f501b

add some tolerance on this test since it fails on 32-bit apparently

[embray]

How's this?

[chapoton]

arando patchbot is 32 bits, and has given a green light

so let us try again

[tmonteil]

I launched a 32bit VM on this.

[tmonteil]

Apparently it is happy too.

[embray]

Thanks!