classification
Title: Implement PEP-3141 for Decimal
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: rejected
Dependencies: 1747 Superseder:
Assigned To: facundobatista Nosy List: facundobatista, gvanrossum, jyasskin, mark.dickinson
Priority: normal Keywords: patch

Created on 2007-12-14 05:35 by jyasskin, last changed 2008-01-13 01:05 by jyasskin. This issue is now closed.

Files
File name Uploaded Description Edit
decimal_3141.patch jyasskin, 2007-12-14 05:35
decimal_3141.patch jyasskin, 2007-12-16 02:02
decimal-3141.patch jyasskin, 2008-01-06 23:18 For py2.6
decimal-3141-just-trunc.patch jyasskin, 2008-01-07 06:56 For py2.6
decimal-3141-just-trunc-registered.patch jyasskin, 2008-01-08 16:25 2.6, faster
Messages (20)
msg58614 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2007-12-14 05:35
I added __round__, __ceil__, __floor__, and __trunc__
msg58623 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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) * (Python committer) 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 "<stdin>", line 1, in <module>
  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 "<stdin>", line 1, in <module>
  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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2007-12-20 15:53
Right. Will do.
msg59417 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2008-01-13 01:05
The discussion on issue 1682 concluded that Decimal should not implement
Real at all.
History
Date User Action Args
2008-01-13 01:05:29jyasskinsetstatus: open -> closed
resolution: rejected
messages: + msg59846
2008-01-08 16:25:54jyasskinsetfiles: + decimal-3141-just-trunc-registered.patch
messages: + msg59542
2008-01-08 04:53:54jyasskinsetmessages: + msg59522
2008-01-08 02:30:52mark.dickinsonsetmessages: + msg59516
2008-01-07 21:33:51mark.dickinsonsetmessages: + msg59496
2008-01-07 06:56:16jyasskinsetfiles: + decimal-3141-just-trunc.patch
messages: + msg59434
versions: + Python 2.6
2008-01-06 23:18:30jyasskinsetfiles: + decimal-3141.patch
dependencies: + issubclass(NotSubclassOfObject, InstanceOfAbcMeta) fails instead of returning False
messages: + msg59417
2008-01-06 22:29:44adminsetkeywords: - py3k
versions: Python 3.0
2007-12-20 15:53:36jyasskinsetmessages: + msg58894
2007-12-20 15:18:26gvanrossumsetmessages: + msg58886
2007-12-20 05:41:34jyasskinsetmessages: + msg58858
2007-12-19 17:23:19gvanrossumsetmessages: + msg58812
2007-12-19 17:06:07facundobatistasetmessages: + msg58810
2007-12-19 04:15:15gvanrossumsetstatus: pending -> open
nosy: + gvanrossum
resolution: postponed -> (no value)
messages: + msg58791
2007-12-18 11:54:00facundobatistasetstatus: open -> pending
resolution: postponed
messages: + msg58738
2007-12-18 05:16:46jyasskinsetmessages: + msg58723
2007-12-17 11:53:27mark.dickinsonsetmessages: + msg58688
2007-12-16 02:02:22jyasskinsetfiles: + decimal_3141.patch
messages: + msg58669
2007-12-14 15:17:46mark.dickinsonsetmessages: + msg58628
2007-12-14 12:13:22mark.dickinsonsetnosy: + mark.dickinson
messages: + msg58623
2007-12-14 10:43:56christian.heimessetpriority: normal
assignee: facundobatista
keywords: + py3k, patch
nosy: + facundobatista
2007-12-14 05:35:04jyasskincreate