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:

Status badges

Description (last modified by Michael Abshoff)

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)

trac3938-coercion-converts-native.patch (4.6 KB) - added by Carl Witty 14 years ago.
3938-type-coercion-2.patch (13.7 KB) - added by Robert Bradshaw 14 years ago.
3938-type-coercion-3.patch (7.3 KB) - added by Robert Bradshaw 14 years ago.
3938.patch (21.4 KB) - added by David Roe 14 years ago.
Merged the three patches, added a few fixes to precision.
3938-type-coercion-final.patch (20.1 KB) - added by Robert Bradshaw 14 years ago.
apply only this patch

Download all attachments as: .zip

Change History (23)

Changed 14 years ago by Carl Witty

comment:1 Changed 14 years ago by Robert Bradshaw

I've been playing around with this a bit, simplified your patch some, but one consequence is that

sage: parent(RealField(100)(1.5) + float(1.5)) # good?
<type 'float'>
sage: RealField(100)(2^4000) == float('inf')   # bad?
True

Thoughts?

comment:2 Changed 14 years ago by Carl Witty

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 Robert Bradshaw

Attachment: 3938-type-coercion-2.patch added

Changed 14 years ago by Robert Bradshaw

Attachment: 3938-type-coercion-3.patch added

comment:3 Changed 14 years ago by Robert Bradshaw

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 Michael Abshoff

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 Arnaud Bergeron

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:6 Changed 14 years ago by Robert Bradshaw

Is this not the desired behavior?

comment:7 Changed 14 years ago by Arnaud Bergeron

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 Michael Abshoff

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 Michael Abshoff

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 David Roe

I didn't find the Heisenbug that Michael mentioned. I rebased the patches against 3.2.3.

comment:11 Changed 14 years ago by David Roe

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 Robert Bradshaw

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 David Roe

Attachment: 3938.patch added

Merged the three patches, added a few fixes to precision.

comment:13 Changed 14 years ago by Craig Citro

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 a TESTS) 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:14 Changed 14 years ago by Robert Bradshaw

OK, I'll go ahead and add those doctests.

Changed 14 years ago by Robert Bradshaw

apply only this patch

comment:15 Changed 14 years ago by Robert Bradshaw

OK, I added the doctests and fixed the indentation.

comment:16 Changed 14 years ago by Craig Citro

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 Michael Abshoff

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 Michael Abshoff

Milestone: sage-3.4.1sage-3.3
Resolution: fixed
Status: newclosed

Merged 3938-type-coercion-final.patch with spelling fix in Sage 3.3.alpha3.

Cheers,

Michael

Note: See TracTickets for help on using tickets.