Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ceil(), floor() and round() broken in Decimal #46997

Closed
mdickinson opened this issue May 3, 2008 · 9 comments
Closed

ceil(), floor() and round() broken in Decimal #46997

mdickinson opened this issue May 3, 2008 · 9 comments
Assignees
Labels
stdlib Python modules in the Lib dir

Comments

@mdickinson
Copy link
Member

BPO 2748
Nosy @rhettinger, @facundobatista, @mdickinson
Files
  • decimal_ceilfloor.patch
  • decimal_ceilfloor2.patch: Revised patch, no keyword arguments to round
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/mdickinson'
    closed_at = <Date 2008-05-09.13:43:52.437>
    created_at = <Date 2008-05-03.19:10:51.446>
    labels = ['library']
    title = 'ceil(), floor() and round() broken in Decimal'
    updated_at = <Date 2008-05-09.13:43:52.383>
    user = 'https://github.com/mdickinson'

    bugs.python.org fields:

    activity = <Date 2008-05-09.13:43:52.383>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2008-05-09.13:43:52.437>
    closer = 'mark.dickinson'
    components = ['Library (Lib)']
    creation = <Date 2008-05-03.19:10:51.446>
    creator = 'mark.dickinson'
    dependencies = []
    files = ['10186', '10188']
    hgrepos = []
    issue_num = 2748
    keywords = ['patch']
    message_count = 9.0
    messages = ['66164', '66165', '66169', '66170', '66187', '66201', '66203', '66238', '66467']
    nosy_count = 4.0
    nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'jyasskin']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue2748'
    versions = ['Python 3.0']

    @mdickinson
    Copy link
    Member Author

    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.

    @mdickinson mdickinson added the stdlib Python modules in the Lib dir label May 3, 2008
    @jyasskin
    Copy link
    Mannequin

    jyasskin mannequin commented May 3, 2008

    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.

    @mdickinson
    Copy link
    Member Author

    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.

    @rhettinger
    Copy link
    Contributor

    Thanks Mark. I'll review your patch when it's ready.

    @mdickinson
    Copy link
    Member Author

    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.

    @rhettinger
    Copy link
    Contributor

    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.

    @mdickinson
    Copy link
    Member Author

    Here's a revised patch, with the following changes

    • no keyword arguments to round
    • check that the second argument to round is an integer.

    @rhettinger
    Copy link
    Contributor

    Thanks for the patch. Please apply.

    @rhettinger rhettinger assigned mdickinson and unassigned rhettinger May 8, 2008
    @mdickinson
    Copy link
    Member Author

    Thanks for reviewing this, Raymond!

    Committed, r62938.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants