Issue 2748: ceil(), floor() and round() broken in Decimal (original) (raw)
Created on 2008-05-03 19:10 by mark.dickinson, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Messages (9)
Author: Mark Dickinson (mark.dickinson) * 
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.
Author: Jeffrey Yasskin (jyasskin) *
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.
Author: Mark Dickinson (mark.dickinson) *
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.
Author: Raymond Hettinger (rhettinger) *
Date: 2008-05-03 20:13
Thanks Mark. I'll review your patch when it's ready.
Author: Mark Dickinson (mark.dickinson) *
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.
Author: Raymond Hettinger (rhettinger) *
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.
Author: Mark Dickinson (mark.dickinson) *
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.
Author: Raymond Hettinger (rhettinger) *
Date: 2008-05-04 20:19
Thanks for the patch. Please apply.
Author: Mark Dickinson (mark.dickinson) *
Date: 2008-05-09 13:43
Thanks for reviewing this, Raymond!
Committed, r62938.
History
Date
User
Action
Args
2022-04-11 14:56:33
admin
set
github: 46997
2008-05-09 13:43:52
mark.dickinson
set
status: open -> closed
messages: +
2008-05-08 17:24:40
rhettinger
set
assignee: rhettinger -> mark.dickinson
2008-05-04 20:19:30
rhettinger
set
resolution: accepted
messages: +
2008-05-04 12:45:53
mark.dickinson
set
files: + decimal_ceilfloor2.patch
messages: +
2008-05-04 11:52:09
rhettinger
set
messages: +
2008-05-04 01:35:24
mark.dickinson
set
files: + decimal_ceilfloor.patch
keywords: + patch
messages: +
2008-05-03 20:13:56
rhettinger
set
messages: +
2008-05-03 20:04:22
mark.dickinson
set
messages: +
2008-05-03 20:02:43
rhettinger
set
assignee: facundobatista -> rhettinger
2008-05-03 19:28:59
jyasskin
set
nosy: + rhettinger
messages: +
2008-05-03 19:10:51
mark.dickinson
create