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: |
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)
Change History (28)
comment:1 Changed 13 years ago by
- Component changed from basic arithmetic to coercion
- Owner changed from somebody to robertwb
comment:2 Changed 13 years ago by
comment:3 Changed 13 years ago by
- 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
- 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
- Keywords editor_mhansen added
comment:6 Changed 13 years ago by
- 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
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
comment:8 Changed 13 years ago by
- 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
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
I agree with cwitty, I'll be taking a look at #3938.
comment:11 follow-up: ↓ 12 Changed 13 years ago by
#3938 has been resolved.
comment:12 in reply to: ↑ 11 Changed 13 years ago by
comment:13 Changed 13 years ago by
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
- 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
- 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
Changed 13 years ago by
comment:16 Changed 13 years ago by
- 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
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
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
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
comment:20 Changed 12 years ago by
- 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
- 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
comment:22 Changed 12 years ago by
- Milestone changed from sage-3.4.1 to sage-3.3
comment:23 Changed 12 years ago by
- 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
comment:25 Changed 12 years ago by
- 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
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.