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

Odd floor-division corner case #66394

Closed
mdickinson opened this issue Aug 14, 2014 · 20 comments
Closed

Odd floor-division corner case #66394

mdickinson opened this issue Aug 14, 2014 · 20 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@mdickinson
Copy link
Member

BPO 22198
Nosy @tim-one, @rhettinger, @mdickinson, @abalkin, @stevendaprano, @encukou, @skrah, @eryksun
Files
  • issue22198.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 = None
    closed_at = <Date 2016-07-07.15:31:52.824>
    created_at = <Date 2014-08-14.16:47:41.777>
    labels = ['interpreter-core', 'type-bug']
    title = 'Odd floor-division corner case'
    updated_at = <Date 2016-07-07.15:31:52.822>
    user = 'https://github.com/mdickinson'

    bugs.python.org fields:

    activity = <Date 2016-07-07.15:31:52.822>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-07-07.15:31:52.824>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2014-08-14.16:47:41.777>
    creator = 'mark.dickinson'
    dependencies = []
    files = ['36568']
    hgrepos = []
    issue_num = 22198
    keywords = ['patch']
    message_count = 20.0
    messages = ['225305', '225309', '225313', '225314', '225315', '225340', '225359', '225360', '225365', '225386', '225401', '226541', '226542', '227307', '228865', '230958', '234068', '234069', '241088', '241342']
    nosy_count = 10.0
    nosy_names = ['tim.peters', 'rhettinger', 'mark.dickinson', 'belopolsky', 'casevh', 'Arfrever', 'steven.daprano', 'petr.viktorin', 'skrah', 'eryksun']
    pr_nums = []
    priority = 'normal'
    resolution = 'wont fix'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22198'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @mdickinson
    Copy link
    Member Author

    I'm not sure it's worth fixing this, but it seems worth recording:

    >>> -0.5 // float('inf')
    -1.0

    I was expecting a value of -0.0, and while IEEE 754 doesn't cover the floor division operation, I'm reasonably confident that that's the value it would have recommended if it had. :-)

    However, it's difficult to come up with a situation where the difference matters: there aren't any obvious invariants I can think of that are broken by this special case. So unless anyone thinks it should be changed, I'll settle for recording the oddity in this issue, and closing as won't fix after a short period.

    @mdickinson mdickinson self-assigned this Aug 14, 2014
    @mdickinson mdickinson added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Aug 14, 2014
    @stevendaprano
    Copy link
    Member

    On Thu, Aug 14, 2014 at 04:47:41PM +0000, Mark Dickinson wrote:

    I'm not sure it's worth fixing this, but it seems worth recording:

    -0.5 // float('inf')
    -1.0

    I was expecting a value of -0.0, and while IEEE 754 doesn't cover
    the floor division operation, I'm reasonably confident that that's the
    value it would have recommended if it had. :-)

    Hmmm. I'm not so sure. -0.5 // something_really_big gives -1:

    py> -0.5//1e200
    -1.0

    Consider something_really_big as it gets bigger and bigger and
    approaches infinity, if we *informally* take the limit -> inf I think it
    makes sense for it to return -1. Another way of looking at it is that
    -0.5/inf returns a negative infinitesimal quantity, and then taking the
    floor returns -1. So I think the current behaviour is "correct", for
    some definition of correct.

    The alternative is a discontinuity, where -0.5//x = -1 for all finite
    but huge x and then suddenly 0 when x overflows to infinity. That's
    probably a bad idea.

    @tim-one
    Copy link
    Member

    tim-one commented Aug 14, 2014

    I'm OK with -1, but I don't get that or -0.0 on 32-bit Windows Py 3.4.1:

    Python 3.4.1 (v3.4.1:c0e311e010fc, May 18 2014, 10:38:22) [MSC v.1600 32 bit (Intel)] on win32
    Type "copyright", "credits" or "license()" for more information.
    >>> -0.5 // float('inf')
    nan

    So maybe NaN is the best answer ;-)

    In favor of -1.0: that _is_ the limit of the mathematical floor(-0.5 / x) as x approaches +infinity.

    In favor of -0.0: it "should be" mathematically that floor_division(x/y) = floor(x / y), and floor(-0.5 / inf) = floor(-0.0) = ... well, not -0.0! floor() in Py3 is defined to return an integer, and there is no -0 integer:

    >>> floor(-0.0)
    0

    That's +0. So I see no justification at all for -0.0 in Py3. -1 seems the best that can be done. The NaN I actually get doesn't make sense.

    @mdickinson
    Copy link
    Member Author

    Steven: there's a set of (unwritten) rules for how the IEEE 754 operations work. (I think they actually were articulated explicitly in some of the 754r drafts, but didn't make it into the final version.) One of them is that ideally, a floating-point operations works as though the corresponding mathematical operation were performed exactly on the inputs (considered as real numbers), followed by a rounding step that takes the resulting real number and rounds it to the nearest floating-point number. This is how essentially all the operations prescribed in IEEE 754 behave, with a greater or lesser amount of hand-waving when it comes to specifying results for special cases like infinities and nans. In this case, the underlying mathematical operation is x, y -> floor(x / y). The only tricky point is the extension to infinity, but we've got the existing behaviour of regular division to guide us there - the result of dividing a finite value by an infinity is an appropriately signed zero. So there's really not a lot of room for manoeuvre in an IEEE 754-like operation.

    The alternative is a discontinuity, where -0.5//x = -1 for all finite
    but huge x and then suddenly 0 when x overflows to infinity. That's
    probably a bad idea.

    Shrug: the underlying mathematical operation is discontinuous; I really don't see a problem here. In any case, if you're worried about discontinuities, what about the one that occurs between positive values and negative values of x in the current implementation (a jump from 0 to -1)? Continuity takes second place to correctness here.

    @mdickinson
    Copy link
    Member Author

    [Tim]
    >>> -0.5 // float('inf')
    nan

    Urk! I wonder what's going on there. I think I like that answer even less than -1.0.

    IEEE 754's floor does indeed take -0.0 to -0.0.

    @rhettinger
    Copy link
    Contributor

    ideally, a floating-point operations works as though the
    corresponding mathematical operation were performed exactly
    on the inputs (considered as real numbers), followed by a rounding
    step that takes the resulting real number and rounds it to the
    nearest floating-point number.

    FWIW, the Decimal Arithmetic Specification was created around the same principle. Accordingly, it gets the answer that Mark expected:

      >>> from decimal import Decimal
      >>> Decimal('-0.5') // Decimal('Inf')
      Decimal('-0')

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Aug 15, 2014

    I think the intention of the standard is pretty much as Mark
    said in msg225314. The fact that decimal behaves that way is
    another indicator, since Cowlishaw really tried to mirror the
    2008 standard as closely as possible.

    @tim-one
    Copy link
    Member

    tim-one commented Aug 15, 2014

    To be clear, I agree -0.0 is "the correct" answer, and -1.0 is at best defensible via a mostly-inappropriate limit argument. But in Py3 floor division of floats returns an integer, and there is no integer -0. Nor, God willing, will there ever be ;-)

    Looks to me like what (Py3's, at least) floatobject.c's floor_divmod() returns (the source of float floor division's result) when the 2nd argument is infinite is largely an accident, depending on what the platform C fmod() and floor() happen to return. So it would require special-casing an infinite denominator in that function to force any specific cross-platform result.

    @eryksun
    Copy link
    Contributor

    eryksun commented Aug 15, 2014

    decimal.Decimal 'floor division' is integer division that truncates toward 0 (see 9.4.2).

        >>> Decimal('-0.5').__floor__()
        -1
        >>> Decimal('-0.5').__floordiv__(1)
       Decimal('-0')

    Numpy 1.8.1:

        >>> np.float32(-0.5) // 1
        -1.0
        >>> np.float32(-0.5) // float('inf')
        -0.0
    
        >>> np.array([-0.5]) // 1
        array([-1.])
        >>> np.array([-0.5]) // float('inf')
        array([-0.])

    @mdickinson
    Copy link
    Member Author

    But in Py3 floor division of floats returns an integer.

    Not in my version!

    Python 3.4.1 (default, May 21 2014, 01:39:38) 
    [GCC 4.2.1 Compatible Apple LLVM 5.1 (clang-503.0.40)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> -3.0 // 5.0
    -1.0

    Maybe I'm using the wrong time machine.

    @tim-one
    Copy link
    Member

    tim-one commented Aug 16, 2014

    Sorry, Mark - I took a true thing and careleslly turned it into a false thing ;-)

    It's math.floor(a_float) that returns an int in Py3, not floor division of floats. So, yup, no real problem with returning -0.0 after all; it's just that it can't be _explained_ via

    x // y means math.floor(x / y)

    is Py3 for float x and y, since the latter returns an int bur the former a float.

    But looks like it can be "explained" via

    x // y means divmod(x, y)[0]

    @encukou
    Copy link
    Member

    encukou commented Sep 7, 2014

    I tried my hand at writing a patch. I hope it is helpful.

    The message of the 2001 commit that introduces this says that "there's no platform-independent way to write a test case for this". I assume with @support.requires_IEEE_754 that is no longer true (at least for non-exotic platforms), or was there another issue?

    I noticed there is no test suite for float floordiv, so I attempted writing a fuller one, but when I saw that
    >>> float('inf') // 1.0
    nan
    I decided to keep my first CPython patch small and focused, so I can learn the ropes. I'll file more issues later.

    @encukou
    Copy link
    Member

    encukou commented Sep 7, 2014

    Note: I signed the contributor agreement form recently, I should have a * soon.

    @abalkin
    Copy link
    Member

    abalkin commented Sep 22, 2014

    I wonder if it would make sense to rewrite float_divmod using the newer POSIX/C99 remquo function. I believe it is designed to compute the exact value of round(x/y), but getting floor instead should not be hard. Its behavior on special values is fully specified.

    From the Linux man-page (I believe POSIX/C99 only guarantees 3 bits in quo):

    NAME
    remquo -- floating-point remainder and quotient function

    SYNOPSIS
         #include <math.h>
    
         double
         remquo(double x, double y, int *quo);
     long double
     remquol(long double x, long double y, int *quo);
    
         float
         remquof(float x, float y, int *quo);

    DESCRIPTION
    The remquo() functions compute the value r such that r = x - n*y, where n is
    the integer nearest the exact value of x/y.

     If there are two integers closest to x/y, n shall be the even one. If r is
     zero, it is given the same sign as x.  This is the same value that is
     returned by the remainder() function.  remquo() also calculates the lower
     seven bits of the integral quotient x/y, and gives that value the same sign
     as x/y. It stores this signed value in the object pointed to by quo.
    

    SPECIAL VALUES
    remquo(x, y, quo) returns a NaN and raises the "invalid" floating-point
    exception if x is infinite or y is 0.

    @encukou
    Copy link
    Member

    encukou commented Oct 9, 2014

    Apologies for the delay; I missed/did not get a notification.

    Alexander, I don't disagree, but I'd like my first patch to Python to not be a refactoring. As I said, I'd like to keep this patch focused. After that I'd like to provide tests the rest of float_divmod; and then perhaps use an entirely different implementation.

    If that's not a good course of action, and you suggest a different one or just tell me to improve everything at once, I will certainly try. But, I think that this patch is an improvement, and that it does fix this bug.

    @encukou
    Copy link
    Member

    encukou commented Nov 10, 2014

    ping, could someone please review the patch?

    @encukou
    Copy link
    Member

    encukou commented Jan 15, 2015

    ping, is there anything I can do to help push the patch forward?

    @mdickinson
    Copy link
    Member Author

    The patch is fine; I just need to find time to look at it properly. That might take a week or two. Sorry for the delay.

    @encukou
    Copy link
    Member

    encukou commented Apr 15, 2015

    ping?

    @mdickinson
    Copy link
    Member Author

    Thanks for the ping, and sorry for forgetting about this.

    I'm -1 on applying this patch. I agree that floor division has some corner case issues (of which this is only one). But there's no clear agreement on what the right answer is, and I don't think making a tiny change to one corner case is worth it in terms of code churn. And making several such tiny changes over the course of different Python releases is something we'd definitely want to avoid.

    Ideally, there'd be a once-and-for-all agreement on exactly what should happen with *all* the corner cases; we'd fix the code to implement exactly that, and then we could forget about it. But without a standard to guide us, I don't think that's going to happen.

    So my vote is to close as "wont fix".

    @mdickinson mdickinson removed their assignment Apr 17, 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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants