Opened 3 years ago
Closed 3 years ago
#23345 closed enhancement (fixed)
conversion for fractions
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage8.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 3 years ago by
 Branch set to u/vdelecroix/23345
 Commit set to 83bf93242d9ffb791034daea87d91701b57eeaae
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 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 3 years ago by
 Commit changed from 83bf93242d9ffb791034daea87d91701b57eeaae to ce7fb25dceab06ce630824ff071ce840b439bd38
Branch pushed to git repo; I updated commit sha1. New commits:
ce7fb25  23345: make it faster

comment:4 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:5 Changed 3 years ago by
 Reviewers set to Marcelo Forets
 Status changed from needs_review to positive_review
code and tests are OK, patchbot is halfgreen.. 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 3 years ago by
 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 3 years ago by
In the doctest, I would prefer to see int(2^100)
instead of (2r)**(100r)
comment:8 Changed 3 years ago by
 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:
e238c9e  address reviewer's comments

comment:9 Changed 3 years ago by
 Reviewers changed from Marcelo Forets to Marcelo Forets, Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:10 Changed 3 years ago by
 Branch changed from u/mforets/23345 to e238c9e321d5f2c46c5553ac75555e9a6f09aef7
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
23345: conversions from Fraction to Rational