#25247 closed defect (fixed)

py3: float.__str__ differences

Reported by: embray Owned by:
Priority: major Milestone: sage-8.3
Component: python3 Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 89f501b (Commits) Commit: 89f501b738f189103eb0ca8aa53f5e16e267692f
Dependencies: Stopgaps:

Description

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?

Change History (25)

comment:1 in reply to: ↑ description Changed 20 months ago by 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).

comment:2 Changed 20 months ago by tscrim

My thoughts are the same as Jeroen's.

comment:3 follow-up: Changed 20 months ago by 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.

comment:4 in reply to: ↑ 3 Changed 20 months ago by jdemeyer

Replying to embray:

It should certainly be documented as a backwards-incompatible change

Since when do we document changes :-)

comment:5 Changed 20 months ago by embray

Oh, right...

comment:6 Changed 20 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-25247
  • Commit set to 5b7da8659f7f8dc44cb7f19d39648436a3617b6e

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:

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

comment:7 Changed 18 months ago by chapoton

Is this needing rewiew ??

comment:8 Changed 18 months ago by chapoton

green bot..

comment:9 Changed 18 months ago by jdemeyer

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

comment:10 Changed 18 months ago by jdemeyer

And you can probably remove __str__ too.

comment:11 Changed 18 months ago by 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?

comment:12 Changed 18 months ago by git

  • Commit changed from 5b7da8659f7f8dc44cb7f19d39648436a3617b6e to 5420f79c71219af5b12d0a8b5a202d20cd88cc9e

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

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

comment:13 Changed 18 months ago by embray

  • Status changed from new to needs_review

Rebased and removed double_str and a couple __str__.

comment:14 Changed 18 months ago by chapoton

Looks good. Jeroen, do you agree ?

comment:15 Changed 18 months ago by 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.

comment:16 Changed 18 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

Then I will give a positive review.

comment:17 Changed 18 months ago by vbraun

  • Status changed from positive_review to needs_work

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

comment:18 Changed 18 months ago by embray

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

comment:19 Changed 18 months ago by git

  • Commit changed from 5420f79c71219af5b12d0a8b5a202d20cd88cc9e to 89f501b738f189103eb0ca8aa53f5e16e267692f

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

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

comment:20 Changed 18 months ago by embray

  • Status changed from needs_work to needs_review

How's this?

comment:21 Changed 18 months ago by chapoton

  • Status changed from needs_review to positive_review

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

so let us try again

comment:22 Changed 18 months ago by tmonteil

I launched a 32bit VM on this.

comment:23 Changed 18 months ago by tmonteil

Apparently it is happy too.

comment:24 Changed 18 months ago by embray

Thanks!

comment:25 Changed 18 months ago by vbraun

  • Branch changed from u/embray/ticket-25247 to 89f501b738f189103eb0ca8aa53f5e16e267692f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.