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

Expose round-to-nearest division algorithm in Objects/longobject.c #53063

Closed
mdickinson opened this issue May 25, 2010 · 6 comments
Closed

Expose round-to-nearest division algorithm in Objects/longobject.c #53063

mdickinson opened this issue May 25, 2010 · 6 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@mdickinson
Copy link
Member

BPO 8817
Nosy @mdickinson, @abalkin
Files
  • div_nearest.patch
  • div_nearest2.patch
  • 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 2010-05-26.16:03:46.463>
    created_at = <Date 2010-05-25.12:56:00.978>
    labels = ['interpreter-core', 'type-feature']
    title = 'Expose round-to-nearest division algorithm in Objects/longobject.c'
    updated_at = <Date 2010-05-26.16:03:46.461>
    user = 'https://github.com/mdickinson'

    bugs.python.org fields:

    activity = <Date 2010-05-26.16:03:46.461>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2010-05-26.16:03:46.463>
    closer = 'mark.dickinson'
    components = ['Interpreter Core']
    creation = <Date 2010-05-25.12:56:00.978>
    creator = 'mark.dickinson'
    dependencies = []
    files = ['17459', '17462']
    hgrepos = []
    issue_num = 8817
    keywords = ['patch']
    message_count = 6.0
    messages = ['106431', '106444', '106462', '106491', '106514', '106540']
    nosy_count = 2.0
    nosy_names = ['mark.dickinson', 'belopolsky']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue8817'
    versions = ['Python 3.2']

    @mdickinson
    Copy link
    Member Author

    The implementation of 'round' for Python integers uses a round-to-nearest form of divmod: a, b -> q, r, where q is the nearest integer to a / b and r = a - b*q.

    This form of divmod would be useful elsewhere. In particular, it's currently needed for implementing multiplication and division of timedeltas by a float: see bpo-1289118 .

    This patch exposes the operation to Python C code as _PyLong_Divmod_Near, and refactors long_round to use this operation.

    @mdickinson mdickinson self-assigned this May 25, 2010
    @mdickinson mdickinson added the type-feature A feature request or enhancement label May 25, 2010
    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    Just a few nitpicks on the patch (in increasing pickiness):

    1. Any reason to prefer PyTuple_SetItem to PyTuple_SET_ITEM at the end of _PyLong_Divmod_Near? You are filling a brand new tuple, so PyTuple_SET_ITEM seems to be more appropriate.

    2. temp = (quo_is_neg ? long_add : long_sub)(..) is clever, but IMO is less readable than

    if (quo_is_neg)
    temp = long_add(..)
    else
    temp = long_sub(..)

    The later form may also be more optimization friendly, particularly if compiler wants to inline static long_add or long_sub.

    1. Given that arguments are named 'a' and 'b' it is a bit confusing to have local variable c of a different type. I think 'cmp' would be a better choice.

    2. I see that you removed a comment that displays round() implemented in python. I think it would be helpful to preserve it for documentation and testing purposes even though the actual algorithm is slightly different. As long as the results are the same, it is helpful to have reference python code.

    3. Similarly to Update Python Software Foundation Copyright Year. #4, having python implementation of divmod_near() displayed somewhere will be helpful.

    @mdickinson
    Copy link
    Member Author

    Thanks for reviewing!

    Updated patch, that addresses points 1-3. For 4, there's no need for the old code, since "self - divmod_near(self, 10**-n)[1]" is enough. And I didn't like the old algorithm anyway; the new one is more straightforward. For 5, I've added a Python version of divmod_near to the comments.

    @abalkin
    Copy link
    Member

    abalkin commented May 25, 2010

    Looking at

        Py_DECREF(one);
     
        result = PyTuple_New(2);
        if (result == NULL)
            goto error;
    ..
      error:
        Py_XDECREF(one);

    If PyTuple_New fails, wouldn't it result in one being DECREF's twice?

    @mdickinson
    Copy link
    Member Author

    Ah, good point. I knew there was a reason I didn't like Py_XDECREF.
    I'll fix this and then apply this patch tonight.

    @mdickinson
    Copy link
    Member Author

    Committed (with the DECREF(one) fix) in r81541.

    @mdickinson mdickinson added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label May 26, 2010
    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants