Opened 2 years ago

Closed 2 years ago

#23345 closed enhancement (fixed)

conversion for fractions

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.0
Component: coercion Keywords:
Cc: mforets Merged in:
Authors: Vincent Delecroix Reviewers: Marcelo Forets, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: e238c9e (Commits) Commit: e238c9e321d5f2c46c5553ac75555e9a6f09aef7
Dependencies: Stopgaps:

Description

We fix py_scalar_parent, py_scalar_to_element and Rational to accept Fractions (from the module fractions).

Change History (10)

comment:1 Changed 2 years ago by vdelecroix

  • Branch set to u/vdelecroix/23345
  • Commit set to 83bf93242d9ffb791034daea87d91701b57eeaae
  • Status changed from new to needs_review

New commits:

83bf93223345: conversions from Fraction to Rational

comment:2 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This is way too slow. Those functions are called a lot in the coercion framework, so they should be as fast as possible. In Cython, checking isinstance(x, Fraction) is about a factor 30 slower than isinstance(x, float)!

I could live with an exact type check like

from fractions import Fraction
cdef type FractionType = <type>Fraction

...
   if type(x) is FractionType:
...

Of course, this won't work with subclasses. I don't know how important that is.

comment:3 Changed 2 years ago by git

  • Commit changed from 83bf93242d9ffb791034daea87d91701b57eeaae to ce7fb25dceab06ce630824ff071ce840b439bd38

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

ce7fb2523345: make it faster

comment:4 Changed 2 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:5 Changed 2 years ago by mforets

  • Reviewers set to Marcelo Forets
  • Status changed from needs_review to positive_review

code and tests are OK, patchbot is half-green.. it doesn't appear to be an error, just that there is a new module added.. Then let me set to positive -- please change if you, Jeroen, plan to make further remarks.

comment:6 Changed 2 years ago by jdemeyer

  • Status changed from positive_review to needs_work

In rational.pyx, I would prefer to move the check for Fraction further down in order not to slow down the conversion RR -> QQ for example.

comment:7 Changed 2 years ago by jdemeyer

In the doctest, I would prefer to see int(2^100) instead of (2r)**(100r)

comment:8 Changed 2 years ago by mforets

  • Branch changed from u/vdelecroix/23345 to u/mforets/23345
  • Commit changed from ce7fb25dceab06ce630824ff071ce840b439bd38 to e238c9e321d5f2c46c5553ac75555e9a6f09aef7
  • Status changed from needs_work to needs_review

New commits:

e238c9eaddress reviewer's comments

comment:9 Changed 2 years ago by jdemeyer

  • Reviewers changed from Marcelo Forets to Marcelo Forets, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:10 Changed 2 years ago by vbraun

  • Branch changed from u/mforets/23345 to e238c9e321d5f2c46c5553ac75555e9a6f09aef7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.