| msg58614 - (view) |
Author: Jeffrey Yasskin (jyasskin) *  |
Date: 2007-12-14 05:35 |
| I added __round__, __ceil__, __floor__, and __trunc__ |
|
|
| msg58623 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2007-12-14 12:13 |
| I think you probably don't want to use quantize here: it makes use of the current context, which means (for example) that trunc(Decimal(integer_with_30_digits)) will fail with an InvalidOperation exception. I assume what's wanted is something that operates purely on Decimals and is unaffected by the context. Is this true? |
|
|
| msg58628 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2007-12-14 15:17 |
| Sorry: that was nonsense. trunc is fine---it's round, floor and ceil that fail on large arguments with this patch: >>> import decimal, math >>> math.floor(decimal.Decimal("1e30")) Traceback (most recent call last): File "", line 1, in File "/Users/dickinsm/python_source/py3k/Lib/decimal.py", line 1475, in __floor__ return trunc(self.quantize(Decimal(1), rounding=ROUND_FLOOR)) File "/Users/dickinsm/python_source/py3k/Lib/decimal.py", line 2265, in quantize 'quantize result has too many digits for current context') File "/Users/dickinsm/python_source/py3k/Lib/decimal.py", line 3546, in _raise_error raise error(explanation) decimal.InvalidOperation: quantize result has too many digits for current context >>> round(decimal.Decimal("1e30")) Traceback (most recent call last): File "", line 1, in File "/Users/dickinsm/python_source/py3k/Lib/decimal.py", line 1487, in __round__ return trunc(self.quantize(Decimal(1), rounding=ROUND_HALF_EVEN)) File "/Users/dickinsm/python_source/py3k/Lib/decimal.py", line 2265, in quantize 'quantize result has too many digits for current context') File "/Users/dickinsm/python_source/py3k/Lib/decimal.py", line 3546, in _raise_error raise error(explanation) decimal.InvalidOperation: quantize result has too many digits for current context >>> Can I suggest using _rescale instead of quantize? For example, def __round__(self, ndigits:_numbers.Integral=None): """Rounds self to ndigits decimal places, defaulting to 0. If ndigits is omitted or None, returns an int, otherwise a Decimal. Rounds half toward even.""" if ndigits is None: return trunc(self._rescale(0, rounding=ROUND_HALF_EVEN)) return self._rescale(-ndigits, rounding=ROUND_HALF_EVEN) |
|
|
| msg58669 - (view) |
Author: Jeffrey Yasskin (jyasskin) * |
Date: 2007-12-16 02:02 |
| Here's a version of the patch that uses _rescale instead of quantize. I don't know enough about how contexts are used to know whether someone might want to know that ceil(Decimal("1e30")) gave a result with more precision than the input. On the other hand, the error .quantize() throws is clearly the wrong one, so this is at least an improvement. |
|
|
| msg58688 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2007-12-17 11:53 |
| Cool! Works for me. I agree that it's not 100% clear that round(large_decimal) should return an integer rather than raising an exception. But, rightly or wrongly, this is what int(large_decimal) does at the moment, and it would be surprising to have int and round behave differently in this respect. The current behaviour also fits with the way that int(large_float) and round(large_float) behave, with a valid integer result returned even if that integer is larger than 2**53. There is of course a problem here that's not present for floats, namely that someone can write round(Decimal("1e1000000")) and then wonder why his/her computer takes so long to give an answer. I don't really see any way around this, other than perhaps a note in the docs. I notice that math.floor(large_float) and math.ceil(large_float) return floats at the moment. Is this something that would change under PEP 3141? If not, should floor(large_decimal) and ceil(large_decimal) return Decimal instances instead of integers? One last thing: would it be worth backporting some of this to Python 2.6, just to avoid unnecessary divergence of the Decimal code between 2.x and 3.0? I guess the trunc() function calls would have to be replaced by calls to the __trunc__ method---would this be a problem? Mark |
|
|
| msg58723 - (view) |
Author: Jeffrey Yasskin (jyasskin) * |
Date: 2007-12-18 05:16 |
| Re math.{floor,ceil}(float): oops, that's definitely a bug. I'll fix it. Re backporting: yes, and I believe trunc() should be backported too. |
|
|
| msg58738 - (view) |
Author: Facundo Batista (facundobatista) * |
Date: 2007-12-18 11:54 |
| The PEP is not approved yet. This look interesting, will take a look again in the future after the PEP approval. Thanks for the work! Regards, |
|
|
| msg58791 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-19 04:15 |
| Sorry, the PEP was approved, just not yet marked as such. :-) Will do so now. |
|
|
| msg58810 - (view) |
Author: Facundo Batista (facundobatista) * |
Date: 2007-12-19 17:06 |
| I'm +1 to this change. As this does not negatively affect the behavior on Py2, for each part of the patch I propose: - decimal.py: incorporate them to the trunk - numbers.py: of course, in Py3 - test_decimal.py: incorporate this tests, activating them if in Py3 What do you think? |
|
|
| msg58812 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-19 17:23 |
| BTW abc.py and _abcoll.py have been backported to 2.6 already, I propose to backport numbers.py and test_abstract_numbers.py as well. |
|
|
| msg58858 - (view) |
Author: Jeffrey Yasskin (jyasskin) * |
Date: 2007-12-20 05:41 |
| Are you guys suggesting the backport before or after checking this in to the py3k branch? Either's fine with me. |
|
|
| msg58886 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-20 15:18 |
| If there aren't too many differences between the 2.6 and 3.0 version of decimal.py and your patch, do 2.6 first, then the next time we merge stuff into 3.0 from the trunk it'll be forward-ported automatically. Though you'd have to start by backporting *numbers.py first. |
|
|
| msg58894 - (view) |
Author: Jeffrey Yasskin (jyasskin) * |
Date: 2007-12-20 15:53 |
| Right. Will do. |
|
|
| msg59417 - (view) |
Author: Jeffrey Yasskin (jyasskin) * |
Date: 2008-01-06 23:18 |
| I think this reflects the consensus of http://mail.python.org/pipermail/python-dev/2008-January/075798.html. I haven't yet implemented Context.round() as I suggested at http://mail.python.org/pipermail/python-dev/2008-January/075920.html because there are more details to discuss, and I don't want to sidetrack the discussion about basic PEP 3141 support. The test currently fails due to issue 1747. I'll double-check that it passes once that's fixed. |
|
|
| msg59434 - (view) |
Author: Jeffrey Yasskin (jyasskin) * |
Date: 2008-01-07 06:56 |
| Much smaller now. 3.0 will need an additional patch beyond the "automatic" forward port. |
|
|
| msg59496 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2008-01-07 21:33 |
| One minor point: Decimals are also created by the _dec_from_triple function: presumably that call to __new__ should also be changed to use super? |
|
|
| msg59516 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2008-01-08 02:30 |
| Some more comments: (1) I don't think I understand the mechanism by which __lt__ and __le__ are supposed to be called. For example, by adding a few print statements it seems that the comparison Decimal(2) > 3 doesn't call __lt__ at any stage. Is this what's expected, or is this unintended? (2) Also on the subject of __le__ and __lt__: if Decimal is going to have rich comparisons (and I guess it is, one way or another :) ) then we might as well get them right for NaNs: IEEE-754(r) says that any comparison involving NaN should return False, with the exception of != which should always return True. With 3-way comparison it wasn't possible to get this right. This would fix Issue 1514428. (Further discussion of this should probably go there---I don't think it should hold up applying this patch...) (3) I'm getting a serious performance hit from this patch: a complete run of test_decimal.py costs me 22.439 seconds (best of 3) with the patch, and 11.604 seconds without. I'm not sure where this is coming from---perhaps it's unavoidable. |
|
|
| msg59522 - (view) |
Author: Jeffrey Yasskin (jyasskin) * |
Date: 2008-01-08 04:53 |
| _dec_from_triple: Probably, yes. It doesn't seem to have any practical effect, but it's good to be consistent. __lt__, __le__: It doesn't matter whether they're called because they have the same effect as __cmp__. They're only there to implement numbers.Real. Perhaps, though, this is a sign that numbers.Real should require __cmp__ instead in 2.6? Or all of the rich comparisons instead of just the two minimal ones? NaNs: I think the guideline is to keep the current behavior for 2.6, and it's not needed for the 3141 implementation, but if that issue comes to a conclusion before I get the 3.0 implementation submitted, I'm happy to follow it. Performance: cProfile points at abc.__instancecheck__ taking 20% of the total time (1132436 calls). This could be a problem... I'll check how that's changed (on my machine) from head and if I can fix it. Thanks for your detailed reviews, Mark! |
|
|
| msg59542 - (view) |
Author: Jeffrey Yasskin (jyasskin) * |
Date: 2008-01-08 16:25 |
| Yes, making Decimal subclass object instead of Real and Inexact makes it as fast as it used to be. ABCMeta.__instancecheck__ is easy to speed up, but after fixing it, we're still about 25% behind. So here'a version that just registers Decimal as a subclass instead of having it inherit. I removed __le__ and __lt__ too, since, without subclassing numbers.Real, nothing requires them to exist, and the behavior's the same. |
|
|
| msg59846 - (view) |
Author: Jeffrey Yasskin (jyasskin) * |
Date: 2008-01-13 01:05 |
| The discussion on issue 1682 concluded that Decimal should not implement Real at all. |
|
|