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

OverflowError from timedelta * float in datetime.py #67709

Closed
abalkin opened this issue Feb 25, 2015 · 20 comments
Closed

OverflowError from timedelta * float in datetime.py #67709

abalkin opened this issue Feb 25, 2015 · 20 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@abalkin
Copy link
Member

abalkin commented Feb 25, 2015

BPO 23521
Nosy @mdickinson, @abalkin, @benjaminp, @serhiy-storchaka
Files
  • issue23521.patch
  • issue23521-2.patch
  • issue23521-3.patch
  • issue23521-4.patch
  • issue23521-5.patch
  • issue23521-6.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/abalkin'
    closed_at = <Date 2015-02-28.15:48:52.917>
    created_at = <Date 2015-02-25.17:05:47.786>
    labels = ['type-bug', 'library']
    title = 'OverflowError from timedelta * float in datetime.py'
    updated_at = <Date 2015-02-28.15:48:52.916>
    user = 'https://github.com/abalkin'

    bugs.python.org fields:

    activity = <Date 2015-02-28.15:48:52.916>
    actor = 'belopolsky'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2015-02-28.15:48:52.917>
    closer = 'belopolsky'
    components = ['Library (Lib)']
    creation = <Date 2015-02-25.17:05:47.786>
    creator = 'belopolsky'
    dependencies = []
    files = ['38234', '38235', '38236', '38237', '38247', '38254']
    hgrepos = []
    issue_num = 23521
    keywords = ['patch']
    message_count = 20.0
    messages = ['236602', '236605', '236613', '236614', '236615', '236623', '236627', '236628', '236629', '236630', '236631', '236632', '236633', '236651', '236681', '236697', '236712', '236717', '236872', '236891']
    nosy_count = 6.0
    nosy_names = ['mark.dickinson', 'belopolsky', 'benjamin.peterson', 'python-dev', 'serhiy.storchaka', 'bdkearns']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue23521'
    versions = ['Python 3.4', 'Python 3.5']

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 25, 2015

    >>> import sys
    >>> sys.modules['_datetime'] = None
    >>> from datetime import timedelta
    >>> timedelta(seconds=1)*0.6112295
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/Users/a/Work/cpython/Lib/datetime.py", line 519, in __mul__
        return self * a / b
      File "/Users/a/Work/cpython/Lib/datetime.py", line 516, in __mul__
        self._microseconds * other)
      File "/Users/a/Work/cpython/Lib/datetime.py", line 411, in __new__
        raise OverflowError("timedelta # of days is too large: %d" % d)
    OverflowError: timedelta # of days is too large: 63720670102

    C implementation is unaffected:

    >>> from datetime import timedelta
    >>> timedelta(seconds=1)*0.6112295
    datetime.timedelta(0, 0, 611229)

    @abalkin abalkin self-assigned this Feb 25, 2015
    @abalkin abalkin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 25, 2015
    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 25, 2015

    Attached patch fixes the issue, but produces a slightly different result:

    >>> timedelta(seconds=1)*0.6112295
    datetime.timedelta(0, 0, 611230)

    Note that C implementation is probably buggy:

    >>> from datetime import *
    >>> timedelta(seconds=1)*0.6112295
    datetime.timedelta(0, 0, 611229)
    >>> timedelta(seconds=0.6112295)
    datetime.timedelta(0, 0, 611230)

    See msg194311 in bpo-8860.

    @serhiy-storchaka
    Copy link
    Member

    You can use self._to_microseconds().

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 25, 2015

    You can use self._to_microseconds().

    Right. Did that and added a simple test.

    @serhiy-storchaka
    Copy link
    Member

    LGTM. But the bug in C implementation should be fixed.

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 25, 2015

    the bug in C implementation should be fixed.

    In the past (see bpo-8860) we were not able to reach a consensus on which behavior is correct and which has a bug:

    >>> timedelta(seconds=1)*0.6112295
    datetime.timedelta(0, 0, 611229)
    >>> timedelta(seconds=0.6112295)
    datetime.timedelta(0, 0, 611230)

    It all boils down to the the fact that

    >>> round(0.6112295*10**6)
    611230

    but

    >>> round(0.6112295, 6) * 10**6
    611229.0

    In my view, timedelta(seconds=1) is 10**6 microseconds and timedelta(seconds=1) * 0.6112295 is 611229.5 microseconds which is correctly rounded to 611230 in timedelta(seconds=0.6112295), but timedelta(seconds=1)*0.6112295 returning 611229 is incorrect.

    If I understand Mark's argument correctly (see msg107097), he views timedeltas as fractional number of seconds rather than integer number of microseconds and in his view timedelta(seconds=0.6112295) is timedelta(seconds=0.6112295) is 0.611229499999999981 seconds because

    >>> Decimal(0.6112295)
    Decimal('0.61122949999999998116351207499974407255649566650390625')

    Thus timedelta(seconds=0.6112295), 0.6112295 should be rounded down to 6 decimal places and result in 611229 microseconds.

    Neither of us thought resolving this was important enough to hold other fixes. In the same spirit, I suggest that we apply my current patch that fixes a crash and pass the rounding curiosity down to the future generations.

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 25, 2015

    I eliminated floating point arithmetics from the calculation completely and it now matches C.

    @bdkearns
    Copy link
    Mannequin

    bdkearns mannequin commented Feb 25, 2015

    Maybe we should also use divide_and_round for __truediv__ to match the C implementation calling divide_nearest there as well?

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 25, 2015

    Maybe we should also use divide_and_round for __truediv__

    You mean in timedelta / float case, right? You are probably right, but can you provide a test case in which it matters?

    @bdkearns
    Copy link
    Mannequin

    bdkearns mannequin commented Feb 25, 2015

    td = timedelta(seconds=1)
    print(td / (1/0.6112295))

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 25, 2015

    >>> td = timedelta(seconds=1)
    >>> print(td / (1/0.6112295))
    0:00:00.611229

    What is wrong with this answer? It is the same as in

    >>> print(td * 0.6112295)
    0:00:00.611229

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 25, 2015

    I've got it:

    >>> import sys
    >>> sys.modules['_datetime'] = None
    >>> from datetime import *
    >>> td = timedelta(seconds=1)
    >>> print(td / (1/0.6112295))
    0:00:00.611230

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 26, 2015

    Fixed truediv in bpo-23521-4.patch.

    @serhiy-storchaka
    Copy link
    Member

    Added comments on Rietveld.

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 26, 2015

    I addressed review comments and fixed the case of negative divisor. There may be a better solution than calling the function recursively, but I wanted an easy to understand code.

    @bdkearns
    Copy link
    Mannequin

    bdkearns mannequin commented Feb 26, 2015

    You saw the demo python implementation of divmod_near in Objects/longobject.c? Maybe it's worth using a common implementation?

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 26, 2015

    You saw the demo python implementation of divmod_near in Objects/longobject.c?

    I was looking for it, but could not find. Thanks for the pointer. (See also bpo-8817.)

    In bpo-23521-6.patch, I've used a slightly optimized variant of divmod_near code and beefed up the tests some more because the code
    is somewhat non-obvious.

    @abalkin
    Copy link
    Member Author

    abalkin commented Feb 26, 2015

    We can further optimize _divide_and_round by changing r*=2 to r<<=1 and q % 2 == 1 to (q & 1), but this will obfuscate the algorithm and limit arguments to ints. (As written, _divide_and_round will work with any numeric types.)

    @serhiy-storchaka
    Copy link
    Member

    LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 28, 2015

    New changeset 8fe15bf68522 by Alexander Belopolsky in branch '3.4':
    Fixes bpo-23521: Corrected pure python implementation of timedelta division.
    https://hg.python.org/cpython/rev/8fe15bf68522

    New changeset d783132d72bc by Alexander Belopolsky in branch 'default':
    Fixes bpo-23521: Corrected pure python implementation of timedelta division.
    https://hg.python.org/cpython/rev/d783132d72bc

    @abalkin abalkin closed this as completed Feb 28, 2015
    @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 type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants