Opened 13 years ago

Closed 12 years ago

#2898 closed defect (fixed)

[with patch, positive review] Coerce integral float and RDF to Integers

Reported by: jason Owned by: robertwb
Priority: major Milestone: sage-3.3
Component: coercion Keywords: editor_mhansen
Cc: Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

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 (3)

2898-jgrout-coerce-to-integer-1.patch (5.0 KB) - added by ncalexan 13 years ago.
2898-jgrout-coerce-to-integer-3.3.alpha2.patch (4.7 KB) - added by cwitty 13 years ago.
trac2898-fixups.patch (4.1 KB) - added by cwitty 12 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 13 years ago by jason

  • Component changed from basic arithmetic to coercion
  • Owner changed from somebody to robertwb

comment:2 Changed 13 years ago 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.

comment:3 Changed 13 years ago 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

comment:4 Changed 13 years ago 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

comment:5 Changed 13 years ago by craigcitro

  • Keywords editor_mhansen added

comment:6 Changed 13 years ago 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).

comment:7 Changed 13 years ago by jason

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

Changed 13 years ago by ncalexan

comment:8 Changed 13 years ago 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.

comment:9 Changed 13 years ago 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.

comment:10 Changed 13 years ago by robertwb

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

comment:11 follow-up: Changed 13 years ago by robertwb

#3938 has been resolved.

comment:12 in reply to: ↑ 11 Changed 13 years ago 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

comment:13 Changed 13 years ago 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.

comment:14 Changed 13 years ago 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.

comment:15 Changed 13 years ago 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

comment:16 Changed 13 years ago by cwitty

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

I've attached two patches, 2898-jgrout-coerce-to-integer-3.3.alpha2.patch and trac2898-fixups.patch. With #3938 plus these two patches over 3.3.alpha2, all doctests pass (except those that were broken in base 3.3.alpha2).

I'm giving a positive review for 2898-jgrout-coerce-to-integer-3.3.alpha2.patch; somebody else should review trac2898-fixups.patch.

comment:17 Changed 12 years ago by jason

I get an error applying the fixups patch:

applying trac2898-fixups.patch?format=raw
patching file sage/rings/real_mpfr.pyx
Hunk #1 FAILED at 396
1 out of 1 hunks FAILED -- saving rejects to file sage/rings/real_mpfr.pyx.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
Errors during apply, please fix and refresh trac2898-fixups.patch?format=raw

comment:18 Changed 12 years ago by jason

I'm confused by the real_mpfr.pyx part of the fixups patch. I can't find the misspelling "cannonical" anywhere in that directory in sage-3.3alpha2.

comment:19 Changed 12 years ago by jason

The real_mpfr.pyx part of the fixups patch should be deleted-mabshoff corrected the typo when merging #3938.

Doctests pass in all files touched by the fixups patch. I'd like someone else to look at the code it touches in free_modules.pyx and verify the changes there. Other than that, this looks good to me.

Changed 12 years ago by cwitty

comment:20 Changed 12 years ago by cwitty

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

We didn't get this in quite soon enough; in alpha5, this gives failing doctests in symbolic/expression.pyx. (The following is hand-edited to remove boring bits of the backtraces.)

sage -t  "devel/sage-mqtmp/sage/symbolic/expression.pyx"    
**********************************************************************
File "/home/cwitty/sage-3.3.alpha5/devel/sage-mqtmp/sage/symbolic/expression.pyx", line 2169:
    sage: S(10.0r).gamma()
Expected:
    362880.000000000
Got:
    362880
**********************************************************************
File "/home/cwitty/sage-3.3.alpha5/devel/sage-mqtmp/sage/symbolic/expression.pyx", line 2180:
    sage: set_verbose(-1); plot(lambda x: S(x).gamma(), -6,4).show(ymin=-3,ymax=3)
Exception raised:
    Traceback (most recent call last):
...
      File "<doctest __main__.example_73[9]>", line 1, in <module>
        set_verbose(-Integer(1)); plot(lambda x: S(x).gamma(), -Integer(6),Integer(4)).show(ymin=-Integer(3),ymax=Integer(3))###line 2180:
    sage: set_verbose(-1); plot(lambda x: S(x).gamma(), -6,4).show(ymin=-3,ymax=3)
      File "/home/cwitty/sage-3.3.alpha5/local/lib/python2.5/site-packages/sage/plot/misc.py", line 428, in wrapper
        return func(*args, **kwds)
...
      File "<doctest __main__.example_73[9]>", line 1, in <lambda>
        set_verbose(-Integer(1)); plot(lambda x: S(x).gamma(), -Integer(6),Integer(4)).show(ymin=-Integer(3),ymax=Integer(3))###line 2180:
    sage: set_verbose(-1); plot(lambda x: S(x).gamma(), -6,4).show(ymin=-3,ymax=3)
      File "expression.pyx", line 2183, in sage.symbolic.expression.Expression.gamma (sage/symbolic/expression.cpp:8410)
    RuntimeError: tgamma_eval(): simple pole
**********************************************************************
File "/home/cwitty/sage-3.3.alpha5/devel/sage-mqtmp/sage/symbolic/expression.pyx", line 2204:
    sage: set_verbose(-1); plot(lambda x: S(x).lgamma(), -7,8, plot_points=1000).show()
Exception raised:
    Traceback (most recent call last):
...
      File "<doctest __main__.example_74[7]>", line 1, in <module>
        set_verbose(-Integer(1)); plot(lambda x: S(x).lgamma(), -Integer(7),Integer(8), plot_points=Integer(1000)).show()###line 2204:
    sage: set_verbose(-1); plot(lambda x: S(x).lgamma(), -7,8, plot_points=1000).show()
...
      File "<doctest __main__.example_74[7]>", line 1, in <lambda>
        set_verbose(-Integer(1)); plot(lambda x: S(x).lgamma(), -Integer(7),Integer(8), plot_points=Integer(1000)).show()###line 2204:
    sage: set_verbose(-1); plot(lambda x: S(x).lgamma(), -7,8, plot_points=1000).show()
      File "expression.pyx", line 2210, in sage.symbolic.expression.Expression.lgamma (sage/symbolic/expression.cpp:8476)
    RuntimeError: lgamma_eval(): logarithmic pole
**********************************************************************
2 items had failures:
   2 of  10 in __main__.example_73
   1 of  10 in __main__.example_74
***Test Failed*** 3 failures.

comment:21 Changed 12 years ago by cwitty

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

I've placed a proposed patch for the above doctest failure at #5199. So I'm now saying that this is "ready for review" again; but the patch now depends on #5199 (and #3938, if the reviewer is using a version of Sage older than 3.3.alpha3).

comment:22 Changed 12 years ago by mabshoff

  • Milestone changed from sage-3.4.1 to sage-3.3

comment:23 Changed 12 years ago 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

The fixups patch looks good to me except for the following two failures:

**********************************************************************
File "/home/mhansen/sage-coxeter/devel/sage-main/sage/rings/number_field/totallyreal_data.pyx", line 74:
    sage: hermite_constant(1) # trivial one-dimensional lattice
Expected:
    1.0
Got:
    1
**********************************************************************
File "/home/mhansen/sage-coxeter/devel/sage-main/sage/rings/number_field/totallyreal_data.pyx", line 80:
    sage: hermite_constant(8) # E_8
Expected:
    2.0
Got:
    2
**********************************************************************

comment:24 Changed 12 years ago by mabshoff

I cannot reproduce the failures Mike saw and I assume #5199 fixed that.

Cheers,

Michael

comment:25 Changed 12 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged 2898-jgrout-coerce-to-integer-3.3.alpha2.patch and trac2898-fixups.patch in Sage 3.3.rc0.

Cheers,

Michael

Note: See TracTickets for help on using tickets.