Opened 5 months ago

Closed 4 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) 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 5 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/functional-py3

comment:2 Changed 5 months ago by jhpalmieri

  • Commit set to 4f88c7cb57778eac0e25eb8d555318aac97d8a79
  • Status changed from new to needs_review

Is this the right approach? We could put a try ... except block instead: something like

try:
    x = builtins.round(x, ndigits)
except TypeError:
    x = builtins.round(float(x), ndigits)

New commits:

4f88c7ctrac 28035: py3 fix for "round" function in misc/functional.py

comment:3 follow-up: Changed 5 months ago by gh-mwageringel

#25827 seems to be related.

comment:4 in reply to: ↑ 3 Changed 5 months ago by jhpalmieri

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 5 months ago by gh-mwageringel

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 5 months ago by chapoton

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

ok, good enough for me.

comment:7 Changed 5 months ago by chapoton

  • Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Markus Wageringel

comment:8 Changed 4 months ago by vbraun

  • Branch changed from u/jhpalmieri/functional-py3 to 4f88c7cb57778eac0e25eb8d555318aac97d8a79
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.