Opened 4 years ago
Closed 4 years ago
#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, GitHub, GitLab) | 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 4 years ago by
comment:2 Changed 4 years ago by
My thoughts are the same as Jeroen's.
comment:3 follow-up: ↓ 4 Changed 4 years ago by
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 4 years ago by
Replying to embray:
It should certainly be documented as a backwards-incompatible change
Since when do we document changes :-)
comment:5 Changed 4 years ago by
Oh, right...
comment:6 Changed 4 years ago by
- 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:
5b7da86 | Fix RealDoubleElement to use float.__repr__ always, instead of float.__str__, and fix all doctests impacted by that change
|
comment:7 Changed 4 years ago by
Is this needing rewiew ??
comment:8 Changed 4 years ago by
green bot..
comment:9 Changed 4 years ago by
Wouldn't it be better to just remove double_str
completely?
comment:10 Changed 4 years ago by
And you can probably remove __str__
too.
comment:11 Changed 4 years ago by
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 4 years ago by
- Commit changed from 5b7da8659f7f8dc44cb7f19d39648436a3617b6e to 5420f79c71219af5b12d0a8b5a202d20cd88cc9e
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
|
comment:13 Changed 4 years ago by
- Status changed from new to needs_review
Rebased and removed double_str
and a couple __str__
.
comment:14 Changed 4 years ago by
Looks good. Jeroen, do you agree ?
comment:15 Changed 4 years ago by
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 4 years ago by
- 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 4 years ago by
- 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 4 years ago by
Any suggestions for fixing that? Would an # abstol
flag be sufficient? I have no way of testing this.
comment:19 Changed 4 years ago by
- Commit changed from 5420f79c71219af5b12d0a8b5a202d20cd88cc9e to 89f501b738f189103eb0ca8aa53f5e16e267692f
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
|
comment:21 Changed 4 years ago by
- 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 4 years ago by
I launched a 32bit VM on this.
comment:23 Changed 4 years ago by
Apparently it is happy too.
comment:24 Changed 4 years ago by
Thanks!
comment:25 Changed 4 years ago by
- Branch changed from u/embray/ticket-25247 to 89f501b738f189103eb0ca8aa53f5e16e267692f
- Resolution set to fixed
- Status changed from positive_review to closed
Replying to embray:
Fine for me.
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).