classification
Title: ceil(), floor() and round() broken in Decimal
Type: Stage:
Components: Library (Lib) Versions: Python 3.0
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: facundobatista, jyasskin, mark.dickinson, rhettinger
Priority: normal Keywords: patch

Created on 2008-05-03 19:10 by mark.dickinson, last changed 2008-05-09 13:43 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
decimal_ceilfloor.patch mark.dickinson, 2008-05-04 01:35
decimal_ceilfloor2.patch mark.dickinson, 2008-05-04 12:45 Revised patch, no keyword arguments to __round__
Messages (9)
msg66164 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-05-03 19:10
In Python 3.0, the Decimal __round__, __ceil__ and __floor__ functions
don't work as intended:  they all return 1, 0, or -1.

This is easy to fix.  The only reason I'm making an issue (literally) of 
it is that I remember some discussion of whether these functions should
be implemented at all for Decimal, but I don't remember what the outcome 
of that discussion was.  Adding Jeffrey to the nosy list in case he 
remembers.

Either all three functions should be removed, or they should be 
corrected and tests should be added for them.
msg66165 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-05-03 19:28
I remember the answer being that they shouldn't be supported, but then I
stopped paying attention and some patches went in bringing Decimal
closer to the Real API again, so I'm not sure if the earlier discussion
still applies. I'm happy to let the decimal maintainers decide.
msg66169 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-05-03 20:04
I've removed __round__, __ceil__ and __floor__ in r62669;  the code was 
undocumented, untested and just plain wrong, so there doesn't seem to be 
any point in leaving it in.  (I'm not quite sure how it got there in the 
first place.)

This still leaves the question of whether to implement these functions.
I can provide a patch if it's desirable.
msg66170 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-05-03 20:13
Thanks Mark.  I'll review your patch when it's ready.
msg66187 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-05-04 01:35
Here's a patch that implements __ceil__, __floor__ and __round__.  (It 
seems that just removing __ceil__, __floor__ and __round__ is not an 
option, as I just discovered when r62669 turned all the buildbots red.)

Points to note:

(1) Two-argument round has essentially the same semantics as quantize.  
To be precise, for a Decimal instance x and an int n,

round(x, n) 

is exactly interchangeable with 

x.quantize(Decimal('1E%s' % -n))

In particular, this means that round uses the rounding mode from the 
current context (which will usually, but not always, be 
ROUND_HALF_EVEN), and that an InvalidOperation exception will be raised 
(or NaN returned) if the rounded value has too many digits for the 
current context precision.

After thinking about it, it seemed better to make the two expressions 
above identical than to have subtle and potentially confusing 
differences between them.

(2) Decimal.__round__ takes two optional arguments, 'context' and 
'rounding', again with exactly the same semantics as the corresponding 
optional arguments for quantize.  At the moment, these arguments aren't 
considered public, and aren't documented.  (And they're only accessible 
through __round__ anyway, not directly through the round() builtin.)

(3) For one-argument round, ceil, and floor, the only real decision to 
be made is what to do with NaNs and infinities.  The spirit of IEEE 
754/854/754r suggests that an attempt to turn an infinity into an 
integer should signal the 'overflow' floating-point exception, while 
turning a NaN into an integer should signal 'invalid';  correspondingly, 
the patch raises OverflowError or ValueError respectively in these 
situations.
msg66201 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-05-04 11:52
The patch basically looks good.  The signature for __round__() should 
not grow beyond the spec for the built-in round() function that calls 
it:  round(x[, n]).  The context and rounding arguments are not part of 
that API.  Just like __int__(), the __round__() method should stick to 
its basic purpose.
msg66203 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-05-04 12:45
Here's a revised patch, with the following changes

 - no keyword arguments to round
 - check that the second argument to round is an integer.
msg66238 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-05-04 20:19
Thanks for the patch.  Please apply.
msg66467 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-05-09 13:43
Thanks for reviewing this, Raymond!

Committed, r62938.
History
Date User Action Args
2008-05-09 13:43:52mark.dickinsonsetstatus: open -> closed
messages: + msg66467
2008-05-08 17:24:40rhettingersetassignee: rhettinger -> mark.dickinson
2008-05-04 20:19:30rhettingersetresolution: accepted
messages: + msg66238
2008-05-04 12:45:53mark.dickinsonsetfiles: + decimal_ceilfloor2.patch
messages: + msg66203
2008-05-04 11:52:09rhettingersetmessages: + msg66201
2008-05-04 01:35:24mark.dickinsonsetfiles: + decimal_ceilfloor.patch
keywords: + patch
messages: + msg66187
2008-05-03 20:13:56rhettingersetmessages: + msg66170
2008-05-03 20:04:22mark.dickinsonsetmessages: + msg66169
2008-05-03 20:02:43rhettingersetassignee: facundobatista -> rhettinger
2008-05-03 19:28:59jyasskinsetnosy: + rhettinger
messages: + msg66165
2008-05-03 19:10:51mark.dickinsoncreate