Opened 12 years ago
Closed 11 years ago
#10736 closed defect (fixed)
int raised to a RealNumber gives an error
Reported by: | Jason Grout | Owned by: | Alex Ghitza |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.2 |
Component: | basic arithmetic | Keywords: | |
Cc: | Dan Drake | Merged in: | sage-4.7.2.alpha0 |
Authors: | Robert Bradshaw | Reviewers: | Dan Drake, Mariah Lenox |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Dan Drake reported this error:
sage: int(1.5)**(2.1) --------------------------------------------------------------------------- TypeError Traceback (most recent call last) /Users/grout/Documents/<ipython console> in <module>() TypeError: unsupported operand type(s) for ** or pow(): 'int' and 'sage.rings.real_mpfr.RealLiteral'
Apply: 10736-real-pow-v2.patch
Attachments (2)
Change History (15)
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
Status: | new → needs_review |
---|
Changed 12 years ago by
Attachment: | 10736-real-pow.patch added |
---|
comment:3 Changed 12 years ago by
This passes all doctests and -- with my minimal knowledge of Cython and MPFR -- the code looks very good. I see that the relevant function automatically coerces to complex numbers; would it make sense to add a doctest for negative ints? And maybe zero, to be complete?
sage: int(-2)^(0.333333) 0.629961522017056 + 1.09112272417509*I sage: int(0)^(1.0) 0.000000000000000 sage: int(0)^(0.0) 1.00000000000000
My guess is that we'll need to test this on a bunch of platforms to suss out any numerical noise. But other than that, I can give this a weak positive review -- we need someone who knows MPFR and Cython better than I do. (Or, I need to learn more of those things...)
comment:4 Changed 12 years ago by
MPFR is platform independent--it's based on gmp/mpir so there's no "numerical noise" issues to worry about as there's no (machine-level) floating point arithmetic.
comment:5 Changed 11 years ago by
My knowledge of Cython and MPFR is even more minimal. But the word "track" is in this ticket, should probably be fixed.
comment:6 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:7 Changed 11 years ago by
I fixed the "track #10736" bit and added the doctests I mentioned above to the "v2" patch.
comment:8 Changed 11 years ago by
Authors: | → Robert Bradshaw |
---|---|
Reviewers: | → Dan Drake |
comment:9 follow-up: 10 Changed 11 years ago by
Dan, anything you think still needs to be reviewed here? That would be helpful for any future reviewers - sounds like you want a Cython guru to check it, correct, otherwise all is well?
comment:10 Changed 11 years ago by
Replying to kcrisman:
Dan, anything you think still needs to be reviewed here? That would be helpful for any future reviewers - sounds like you want a Cython guru to check it, correct, otherwise all is well?
Yep, just a quick once-over by someone who knows Cython and maybe a bit about MPFR. The code is simple and I can see it's very reasonable (and it fixes the problem!), but I'd like another person to look at it.
comment:11 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Reviewers: | Dan Drake → Dan Drake, Mariah Lenox |
Status: | needs_review → positive_review |
Applied patch to sage-4.7.1.alpha2, did 'sage -b' then 'make testlong'. The patch fixed the reported problem. All tests passed. Positive review!
comment:12 Changed 11 years ago by
Milestone: | sage-4.7.1 → sage-4.7.2 |
---|
comment:13 Changed 11 years ago by
Merged in: | → sage-4.7.2.alpha0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Note that this works properly for rationals:
Also, this thing is likely to happen in list comprehensions using
range()
, since that returns Python ints. Right now, this doesn't work as expected: