Ticket #2898 (new defect)

Opened 8 months ago

Last modified 2 months ago

[with patch, needs work] Coerce integral float and RDF to Integers

Reported by: jason Assigned to: robertwb
Priority: major Milestone: sage-3.2.2
Component: coercion Keywords: editor_mhansen
Cc:

Description 

>  That said, William, is there a reason why this doesn't work?  This is
> >  what is necessitating the two type conversions above.
> >
> >  sage: Integer(float(2))
> >
> > ---------------------------------------------------------------------------
> >  <type 'exceptions.TypeError'>             Traceback (most recent call last)
> >
> >  /home/grout/<ipython console> in <module>()
> >
> >  /home/grout/integer.pyx in sage.rings.integer.Integer.__init__()
> >
> >  <type 'exceptions.TypeError'>: unable to coerce element to an integer
> >
> >
> >  sage: Integer(RDF(2))
> >
> > ---------------------------------------------------------------------------
> >  <type 'exceptions.TypeError'>             Traceback (most recent call last)
> >
> >  /home/grout/<ipython console> in <module>()
> >
> >  /home/grout/integer.pyx in sage.rings.integer.Integer.__init__()
> >
> >  <type 'exceptions.TypeError'>: unable to coerce element to an integer
> >
> >
> >  I guess I would think it was a design decision to not convert floating
> >  points to ints automatically.  However, the following does work:
> >
> >  sage: Integer(RR(2))
> >  2
> >
> >
> >  This seems inconsistent.

Yep.  I think it's just a NotImplementedError.  Please implement it
and post a patch.  Make sure that it only succeeds if

   Integer(k(a)) == a

and otherwise fails.  I.e., Integer(k(a)) should *not* truncate k(a).

William

Attachments

2898-jgrout-coerce-to-integer-1.patch (5.0 kB) - added by ncalexan on 08/13/2008 05:33:50 PM.

Change History

04/12/2008 09:09:44 AM changed by jason

  • owner changed from somebody to robertwb.
  • component changed from basic arithmetic to coercion.

04/12/2008 09:40:32 PM changed by jason

I've put up a preliminary patch at:

http://sage.math.washington.edu/home/jason/trac-2898-coerce-to-Int.patch

This is undergoing doctesting in alpha3 right now.

04/13/2008 12:00:16 PM changed by mabshoff

  • summary changed from Coerce float and RDF to Integers to [with preliminary patch, not ready for review?] Coerce float and RDF to Integers.

06/09/2008 12:01:21 AM changed by mabshoff

  • summary changed from [with preliminary patch, not ready for review?] Coerce float and RDF to Integers to [with preliminary patch, not ready for review?] Coerce integral float and RDF to Integers.

Jason,

any update on this patch? I corrected the subject to be a little clearer.

Cheers,

Michael

06/19/2008 09:47:46 PM changed by craigcitro

  • keywords set to editor_mhansen.

06/23/2008 05:27:50 PM changed by jason

  • summary changed from [with preliminary patch, not ready for review?] Coerce integral float and RDF to Integers to [with preliminary patch, needs doctest checking] Coerce integral float and RDF to Integers.

I believe the patch is ready to be doctested (which is where my time gave out before).

06/23/2008 05:29:39 PM changed by jason

This is supposed to fix #2899, so it would be good to check if the other issue is resolved as well.

08/13/2008 05:33:50 PM changed by ncalexan

  • attachment 2898-jgrout-coerce-to-integer-1.patch added.

08/13/2008 05:34:51 PM changed by ncalexan

  • summary changed from [with preliminary patch, needs doctest checking] Coerce integral float and RDF to Integers to [with patch, needs review] Coerce integral float and RDF to Integers.

The patch I posted is basically the same as jgrout's; it sat in my tree for a long time without any problems.

It looks good to me, but let's get a second opinion.

08/23/2008 02:15:13 PM changed by cwitty

Unfortunately, this patch gives: 

sage: 1.0r/8
1/8

which is IMHO unacceptable.

I think that the bug is actually in the coercion framework, rather than in this patch; so I've opened a new issue for that bug at #3938. Once it is fixed, then we can revisit this patch.

08/23/2008 03:36:43 PM changed by robertwb

I agree with cwitty, I'll be taking a look at #3938.

(follow-up: ↓ 12 ) 08/30/2008 12:39:10 PM changed by robertwb

#3938 has been resolved.

(in reply to: ↑ 11 ) 08/30/2008 04:03:54 PM changed by mabshoff

Replying to robertwb:

#3938 has been resolved.

Hi Robert,

so if I understand you correctly this patch here should not be merged since it fixes a symptom and not the root cause.

Cheers,

Michael

08/30/2008 08:27:46 PM changed by robertwb

This patch should be applied. The problem is that it exposed a bug that was worse than what it fixed. Now the bug has been resolved, they both should be applied.

09/18/2008 05:36:21 PM changed by mhansen

  • summary changed from [with patch, needs review] Coerce integral float and RDF to Integers to [with patch, positive review] Coerce integral float and RDF to Integers.

Looks good to me. This depends on #3938.

09/18/2008 07:56:21 PM changed by mabshoff

  • summary changed from [with patch, positive review] Coerce integral float and RDF to Integers to [with patch, needs work] Coerce integral float and RDF to Integers.

The patch no longer applies cleanly and also causes a doctest failure (sorry, forgot the details and no longer have the logs ;)). The rebase should be easy since hunk 2 of integer.pyx is affected and it is only a doctest issue.

Cheers,

Michael