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

float is missing __ceil__() and __floor__(), required by numbers.Real #82810

Closed
bluetech mannequin opened this issue Oct 29, 2019 · 11 comments
Closed

float is missing __ceil__() and __floor__(), required by numbers.Real #82810

bluetech mannequin opened this issue Oct 29, 2019 · 11 comments
Labels
3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@bluetech
Copy link
Mannequin

bluetech mannequin commented Oct 29, 2019

BPO 38629
Nosy @mdickinson, @vstinner, @serhiy-storchaka, @vedgar, @isidentical, @bluetech
PRs
  • bpo-38629: implement __floor__ and __ceil__ for float #16985
  • 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 2019-12-15.22:01:27.781>
    created_at = <Date 2019-10-29.10:18:17.895>
    labels = ['interpreter-core', 'type-bug', 'library', '3.9']
    title = 'float is missing __ceil__() and __floor__(), required by numbers.Real'
    updated_at = <Date 2019-12-15.22:01:27.774>
    user = 'https://github.com/bluetech'

    bugs.python.org fields:

    activity = <Date 2019-12-15.22:01:27.774>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-12-15.22:01:27.781>
    closer = 'vstinner'
    components = ['Interpreter Core', 'Library (Lib)']
    creation = <Date 2019-10-29.10:18:17.895>
    creator = 'bluetech'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 38629
    keywords = ['patch']
    message_count = 11.0
    messages = ['355642', '355648', '355650', '355668', '355670', '355673', '355675', '355722', '355744', '358452', '358453']
    nosy_count = 6.0
    nosy_names = ['mark.dickinson', 'vstinner', 'serhiy.storchaka', 'veky', 'BTaskaya', 'bluetech']
    pr_nums = ['16985']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue38629'
    versions = ['Python 3.9']

    @bluetech
    Copy link
    Mannequin Author

    bluetech mannequin commented Oct 29, 2019

    The numbers.Real ABC requires the __ceil__ and __floor__ methods (https://github.com/python/cpython/blob/v3.8.0/Lib/numbers.py#L178-L186), however, the float type does not have them.

    In regular Python, this is not a problem, because math.ceil() and math.floor() special-case float and work on it directly, and numbers.Real is implemented on float by explicitly registering it (so it's not checked that all required methods are implemented).

    Where it becomes a (minor) problem is in typeshed, where the type checker *does* check that all abstract methods are implemented. So, because float is missing these two, typeshed cannot have float implement numbers.Real.

    Refs: python/typeshed#3195

    @bluetech bluetech mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Oct 29, 2019
    @isidentical
    Copy link
    Sponsor Member

    Ran, do you want to work on this or can i take it?

    @bluetech
    Copy link
    Mannequin Author

    bluetech mannequin commented Oct 29, 2019

    You can take it - thanks!

    @serhiy-storchaka
    Copy link
    Member

    If float.__ceil__ is only needed for typeshed, maybe add a special case for typeshed?

    I have doubts about adding a C code which is never even executed.

    @mdickinson
    Copy link
    Member

    I have doubts about adding a C code which is never even executed.

    My reading of the PR is that it *would* be executed: the math module first looks for the __floor__ special method, then falls back to using the libm floor if that doesn't exist. Am I missing something?

    @serhiy-storchaka
    Copy link
    Member

    Oh, you are right. I misunderstood the original issue and thought that the code first checks PyFloat_Check() or PyFloat_CheckExact().

    If float.__ceil__ will be executed it will add significant overhead for creating and executing the method object.

    @mdickinson
    Copy link
    Member

    If float.__ceil__ will be executed it will add significant overhead for creating and executing the method object.

    Yes, I'd definitely like to see timings; I think Victor already asked for those on the PR.

    @isidentical
    Copy link
    Sponsor Member

    $ ./python -m pyperf timeit -s "from math import floor" --duplicate 100 "floor(12345.6)"
    Before:  Mean +- std dev: 52.5 ns +- 2.6 ns
    After:  Mean +- std dev: 71.0 ns +- 1.7 ns
    
    $ ./python -m pyperf timeit -s "from math import ceil" --duplicate 100 "ceil(12345.6)"
    Before:  Mean +- std dev: 51.2 ns +- 1.5 ns
    After:  Mean +- std dev: 74.4 ns +- 2.2 ns

    @vedgar
    Copy link
    Mannequin

    vedgar mannequin commented Oct 31, 2019

    However, this is an instance of a general problem: whenever we want to strongly type (via dunders) protocols that specialcase builtin types, we will have to choose between three options:

    • special case them also in typing engine, complicating the typing engine
    • implement dummy dunders, puzzling readers of the code
    • implement dunders that do the right thing but never actually execute, puzzling Serhiy (and probably others)
    • implement dunders that are actually called (un-specialcasing builtin types), slowing down the common path

    Do we have a preference for a "default" position when we encounter such problems in the future? Of course, we can override it on a case-by-case basis in the presence of good arguments, but still, a default would be nice to have.

    I don't know much about static typing (which is why I loved Python until this typing craze happened:), but it seems to me that we might have another option: we can currently say that a type might be a virtual subclass of an abstract class in more than one way, right? For example, we still support old-style iterators (via __getitem__ and IndexError), IIRC. So, can we say that a type can implement numbers.Real also in two ways: by having some dunders, or by being (a literal or a subtype of) float?

    @vstinner
    Copy link
    Member

    New changeset cb8b946 by Victor Stinner (Batuhan Taşkaya) in branch 'master':
    bpo-38629: implement __floor__ and __ceil__ for float type (GH-16985)
    cb8b946

    @vstinner
    Copy link
    Member

    Thanks Batuhan Taşkaya for the implementation, and thanks Ran Benita for the feature request :-)

    @vstinner vstinner added the 3.9 only security fixes label Dec 15, 2019
    @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
    3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants