Opened 21 months ago
Closed 20 months ago
#28035 closed defect (fixed)
py3 fix in misc/functional.py
Reported by: | jhpalmieri | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.9 |
Component: | python3 | Keywords: | |
Cc: | chapoton | Merged in: | |
Authors: | John Palmieri | Reviewers: | Frédéric Chapoton, Markus Wageringel |
Report Upstream: | N/A | Work issues: | |
Branch: | 4f88c7c (Commits, GitHub, GitLab) | Commit: | 4f88c7cb57778eac0e25eb8d555318aac97d8a79 |
Dependencies: | Stopgaps: |
Description
The round
function in sage/misc/functional.py
fails several tests with Python 3. The output is supposed to be double-precision, so we just convert the input to a float
first.
Change History (8)
comment:1 Changed 21 months ago by
- Branch set to u/jhpalmieri/functional-py3
comment:2 Changed 21 months ago by
- Commit set to 4f88c7cb57778eac0e25eb8d555318aac97d8a79
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 21 months ago by
#25827 seems to be related.
comment:4 in reply to: ↑ 3 Changed 21 months ago by
Replying to gh-mwageringel:
#25827 seems to be related.
Yes, although #25827 is broader in scope, with some questions that need answering. This change is much more focused.
comment:5 Changed 21 months ago by
Ok, fair enough. It looks like this should work for now. Converting to float seems to be what is happening in Python 2 as well, as a naïve test shows:
sage: from six.moves import builtins sage: builtins.round(i, 2) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-5-8b1e2e3412b1> in <module>() ----> 1 builtins.round(i, Integer(2)) /Applications/SageMath/local/lib/python2.7/site-packages/sage/symbolic/expression.pyx in sage.symbolic.expression.Expression.__float__ (build/cythonized/sage/symbolic/expression.cpp:11079)() 1426 raise 1427 except TypeError: -> 1428 raise TypeError("unable to simplify to float approximation") 1429 return ret 1430 TypeError: unable to simplify to float approximation
In the long run, this should probably be changed, so that classes can implement a custom __round__()
without converting to float, such as matrices. However, currently nothing in Sage implements __round__()
– so this seems to be something #25827 will deal with. (Only rings/rational defines it, but it is essentially broken with Python 3.)
I do not quite feel competent enough to set this to positive though, so hopefully someone more knowledgeable can help.
comment:6 Changed 20 months ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
ok, good enough for me.
comment:7 Changed 20 months ago by
- Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Markus Wageringel
comment:8 Changed 20 months ago by
- Branch changed from u/jhpalmieri/functional-py3 to 4f88c7cb57778eac0e25eb8d555318aac97d8a79
- Resolution set to fixed
- Status changed from positive_review to closed
Is this the right approach? We could put a
try ... except
block instead: something likeNew commits:
trac 28035: py3 fix for "round" function in misc/functional.py