Opened 14 years ago
Closed 14 years ago
#3938 closed defect (fixed)
[with patch, with positive review] coercion framework converts built-in types to Sage types when it should not
Reported by: | Carl Witty | Owned by: | Robert Bradshaw |
---|---|---|---|
Priority: | major | Milestone: | sage-3.3 |
Component: | coercion | Keywords: | |
Cc: | Carl Witty | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This came up while reviewing #2898, which adds a conversion from float to ZZ (for integral values). After applying that patch, you get:
sage: 1.0r/8 1/8
That's because of this code in coerce.pyx, which does a conversion rather than a coercion:
elif PY_IS_NUMERIC(x): try: x = yp(x) if PY_TYPE_CHECK(yp, type): return x,y
I tried to fix this, but every time I fixed something it broke something else. I'm going to attach my latest non-working patch, which may or may not be a useful place to start.
[Once this ticket has been reviewed and merged #2898 should be closed]
Attachments (5)
Change History (23)
Changed 14 years ago by
Attachment: | trac3938-coercion-converts-native.patch added |
---|
comment:1 Changed 14 years ago by
comment:2 Changed 14 years ago by
Both of these changes make float act more like RDF. I've sometimes wished that comparison operators would coerce to more-precise types instead of less-precise, but that would be a huge change across big parts of Sage; unless such a policy change is made, I think that both consequences are actually good.
Changed 14 years ago by
Attachment: | 3938-type-coercion-2.patch added |
---|
Changed 14 years ago by
Attachment: | 3938-type-coercion-3.patch added |
---|
comment:3 Changed 14 years ago by
Summary: | coercion framework converts built-in types to Sage types when it should not → [with patch, needs review] coercion framework converts built-in types to Sage types when it should not |
---|
I feel your pain...what a nasty patch to try and write! Well, I finally feel like I've got a correct, working solution. Apply all three patches.
comment:4 Changed 14 years ago by
Cc: | Carl Witty added |
---|---|
Description: | modified (diff) |
Carl,
any chance you could review those three patches?
Cheers,
Michael
comment:5 Changed 14 years ago by
I get
sage: parent(RealField(10)(1) * float(1)) Real Field with 10 bits of precision
with the patches applied against 3.1.2.alpha4.
comment:7 Changed 14 years ago by
Summary: | [with patch, needs review] coercion framework converts built-in types to Sage types when it should not → [with patch, positive review] coercion framework converts built-in types to Sage types when it should not |
---|
I thought the goal was to convert to the type with most precision. It seems I was mistaken and this is trying to convert to the type with less precision.
In that case it works and passes a good chunk of the test suite. (I can't test the rest because of the interfaces/lisp.py failure)
It also passes its own tests and the code looks good.
comment:8 Changed 14 years ago by
This patch causes a Heisenbug:
mabshoff@sage:/scratch/mabshoff/release-cycle/sage-3.1.3.alpha0$ ./sage -t -long devel/sage/sage/modular/modsym/ambient.py sage -t -long devel/sage/sage/modular/modsym/ambient.py ------------------------------------------------------------ Unhandled SIGSEGV: A segmentation fault occured in SAGE. This probably occured because a *compiled* component of SAGE has a bug in it (typically accessing invalid memory) or is not properly wrapped with _sig_on, _sig_off. You might want to run SAGE under gdb with 'sage -gdb' to debug this. SAGE will now terminate (sorry). ------------------------------------------------------------ A mysterious error (perphaps a memory error?) occurred, which may have crashed doctest. [4.6 s] exit code: 768
It does not happen without "-long" and when running "-long -verbose" it also seems to pass. I guess it is time to valgrind :)
Cheers,
Michael
comment:9 Changed 14 years ago by
Summary: | [with patch, positive review] coercion framework converts built-in types to Sage types when it should not → [with patch, needs work] coercion framework converts built-in types to Sage types when it should not |
---|
comment:10 Changed 14 years ago by
I didn't find the Heisenbug that Michael mentioned. I rebased the patches against 3.2.3.
comment:11 Changed 14 years ago by
Summary: | [with patch, needs work] coercion framework converts built-in types to Sage types when it should not → [with patch, needs review] coercion framework converts built-in types to Sage types when it should not |
---|
comment:12 Changed 14 years ago by
Thanks for rebasing this. Since you're not the one who originally wrote it, do you want to give it a review?
Changed 14 years ago by
Attachment: | 3938.patch added |
---|
Merged the three patches, added a few fixes to precision.
comment:13 Changed 14 years ago by
Summary: | [with patch, needs review] coercion framework converts built-in types to Sage types when it should not → [with patch, with positive review pending minor fixes] coercion framework converts built-in types to Sage types when it should not |
---|
Patch looks good. Thankfully, David folded everything into one patch.
I have two minor issues, and after these are fixed, I'm happy to give this a positive review.
- There are two long blocks (an
EXAMPLES
and aTESTS
) that are not indented correctly.
- There are three functions that are moved and one that is new which need doctests. (The moved functions don't necessarily have to have them added, but since it's three functions, it seems worth just adding doctests.)
Once these are done, I'm happy to give this a positive review.
comment:16 Changed 14 years ago by
Summary: | [with patch, with positive review pending minor fixes] coercion framework converts built-in types to Sage types when it should not → [with patch, with positive review] coercion framework converts built-in types to Sage types when it should not |
---|
Looks good.
comment:17 Changed 14 years ago by
One fix:
TypeError: no cannonical coercion from Real Field with 53 bits of precision to Real Field with 100 bits of precision
needs to become
TypeError: no canonical coercion from Real Field with 53 bits of precision to Real Field with 100 bits of precision
I am fixing that in the patch I am applying.
Cheers,
Michael
comment:18 Changed 14 years ago by
Milestone: | sage-3.4.1 → sage-3.3 |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Merged 3938-type-coercion-final.patch with spelling fix in Sage 3.3.alpha3.
Cheers,
Michael
I've been playing around with this a bit, simplified your patch some, but one consequence is that
Thoughts?