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

Add float.is_finite is_nan is_infinite to match Decimal methods #63042

Closed
stevendaprano opened this issue Aug 26, 2013 · 19 comments
Closed

Add float.is_finite is_nan is_infinite to match Decimal methods #63042

stevendaprano opened this issue Aug 26, 2013 · 19 comments
Assignees
Labels
3.7 (EOL) end of life type-feature A feature request or enhancement

Comments

@stevendaprano
Copy link
Member

BPO 18842
Nosy @rhettinger, @facundobatista, @mdickinson, @vstinner, @tiran, @stevendaprano, @vadmium, @serhiy-storchaka, @Mariatta, @corona10
Files
  • float_is.patch
  • issue18842.patch
  • rm-finite.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/vadmium'
    closed_at = <Date 2020-01-22.04:16:51.033>
    created_at = <Date 2013-08-26.15:59:17.227>
    labels = ['type-feature', '3.7']
    title = 'Add float.is_finite is_nan is_infinite to match Decimal methods'
    updated_at = <Date 2020-01-22.04:16:51.030>
    user = 'https://github.com/stevendaprano'

    bugs.python.org fields:

    activity = <Date 2020-01-22.04:16:51.030>
    actor = 'corona10'
    assignee = 'martin.panter'
    closed = True
    closed_date = <Date 2020-01-22.04:16:51.033>
    closer = 'corona10'
    components = []
    creation = <Date 2013-08-26.15:59:17.227>
    creator = 'steven.daprano'
    dependencies = []
    files = ['32089', '32090', '46451']
    hgrepos = []
    issue_num = 18842
    keywords = ['patch']
    message_count = 19.0
    messages = ['196219', '199680', '199696', '199699', '199704', '199705', '203061', '203122', '286434', '286441', '286442', '286446', '286447', '286449', '286462', '286476', '286515', '286842', '360447']
    nosy_count = 10.0
    nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'vstinner', 'christian.heimes', 'steven.daprano', 'martin.panter', 'serhiy.storchaka', 'Mariatta', 'corona10']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue18842'
    versions = ['Python 3.7']

    @stevendaprano
    Copy link
    Member Author

    On bpo-15544 Mark Dickinson suggested adding methods to float to match methods on Decimal, giving type-agnostic ways of testing real numbers that don't rely on converting to float. I don't see any sign that Mark raised a feature request, so I'm taking the liberty of doing so myself.

    Note that the math.is* functions convert to float first, which means that they behave differently. Example: math.isnan(Decimal('sNAN')) raises ValueError, rather than returning True.

    float.is_nan
    float.is_infinity
    float.is_finite

    would mirror the spelling of Decimal methods, rather than the math module. As float doesn't support signalling NANs, there probably isn't any need to include is_snan and is_qnan.

    For what it's worth, I have code that would use this. I currently write something like:

    if isinstance(x, Decimal) and x.is_nan() or math.isnan(x): ...

    in order to prevent triggering the ValueError on signalling NANs.

    @stevendaprano stevendaprano added the type-feature A feature request or enhancement label Aug 26, 2013
    @mdickinson
    Copy link
    Member

    +1 from me. You want float.is_infinite rather than float.is_infinity. is_signed is another one that may be worth considering.

    @tiran
    Copy link
    Member

    tiran commented Oct 13, 2013

    The code is already there so the patch is really small.

    http://hg.python.org/cpython/annotate/5bc7b20dc04a/Objects/floatobject.c#l1046

    I love my time machine. *g*

    @tiran
    Copy link
    Member

    tiran commented Oct 13, 2013

    Here is a longer patch that also adds the methods to int and numbers.Real. It comes with tests and doc updates, too.

    @serhiy-storchaka
    Copy link
    Member

    Fraction? complex? Complex? Integral? Number?

    @tiran
    Copy link
    Member

    tiran commented Oct 13, 2013

    Pardon?

    The methods could be added to complex, too. cmath implements the methods as:

    is_finite:
    Py_IS_FINITE(z.real) && Py_IS_FINITE(z.imag)
    is_infinite:
    Py_IS_INFINITY(z.real) || Py_IS_INFINITY(z.imag)
    is_nan:
    Py_IS_NAN(z.real) || Py_IS_NAN(z.imag)

    For numbers.Real: We can't make the methods abstractmethods because it would be an incompatible change.

    @serhiy-storchaka
    Copy link
    Member

    Should we add these methods to other concrete Number subclasses (as Fraction and complex)?

    @stevendaprano
    Copy link
    Member Author

    On Sat, Nov 16, 2013 at 04:33:36PM +0000, Serhiy Storchaka wrote:

    Should we add these methods to other concrete Number subclasses (as Fraction and complex)?

    Seems like a good idea to me. Is it worth making them part of the Number
    ABC, or is that too much of a change?

    @vadmium
    Copy link
    Member

    vadmium commented Jan 29, 2017

    FWIW, here is an attempt to add Argument Clinic to the Objects/floatobject.c and Objects/longobject.c implementations:

    https://bugs.python.org/file33943/issue20185_conglomerate_v4.diff
    https://bugs.python.org/file33989/clinic_longobject_v3.patch

    If the methods are rejected, the dead code should be removed to avoid wasting people’s time.

    Why do you name the methods is_finite() etc with underscores, when the existing methods math.isfinite() etc do not have underscores? Seems it would add unnecessary confusion.

    @stevendaprano
    Copy link
    Member Author

    On Sun, Jan 29, 2017 at 08:23:05AM +0000, Martin Panter wrote:

    Why do you name the methods is_finite() etc with underscores, when the
    existing methods math.isfinite() etc do not have underscores? Seems it
    would add unnecessary confusion.

    The idea is to enable duck-typing between float and Decimal. Instead of:

    if isinstance(x, float):
        math.isfinite(x)
    else:
        x.is_finite()

    we can just say

    x.is_finite() 

    on both floats and decimals.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 29, 2017

    Of course, somehow I missed that

    @mdickinson
    Copy link
    Member

    @martin: the dead code should definitely be removed from floatobject.c and longobject.c. (Though that's kinda independent of this issue.)

    @rhettinger
    Copy link
    Contributor

    I would like to caution against expansion of core APIs in cases where we already have a way to do it. In almost every corner, Python's APIs are growing, resulting in the language becoming massive, where very few people on the planet can claim to know what is in it and it is becoming much harder to teach as it gets bigger.

    Also, we should place very little value on anecdotal evidence from one core developer who says he once wrote a single line of code that would have benefitted from a single part of the proposed changes. In general, core devs tend to write much more generic code than end users, so our needs are often atypical and do not reflect user needs (in this case, no non-core-dev numeric user has ever requested this behavior or had a real use case that couldn't be met by the existing APIs). The bar for new APIs is much higher than "I wrote a line of code once".

    We should be looking for APIs additions that significantly reduce complexity or solve problems that can't easily be met with existing APIs. Minor expansions create long term maintenance burdens, increase the size of docs making them less usable, cause other implementations to have to follow suit, cause subclassers and users of the numeric module to have to implement additional methods, and make Python more difficult to learn. There is a reason that the zen expresses a preference for only one way to do it.

    Replicating parts the decimal API should always be suspect as well. In general, that module is much more difficult to use and learn than regular floats. Many parts of the API are simply worthless and are there only because they were part of the spec. We have no evidence that any of these proposed methods are actually being used and are of real benefit to real users.

    Also, remember that Decimal is not registered as a Real for a reason. Guido does not consider them to be interoperable with binary floats.

    @serhiy-storchaka
    Copy link
    Member

    I concur with Raymond. This expands the API too much. Not just the float API, but the API of all numeric classes, including third-party classes. And since the existence of additional method in third-party classes (e.g. NumPy classes) can't be guarantied, this couldn't help in the case of msg286441.

    The less harmful way is making math.isfinite() and like supporting non-float numeric types. Always return False for integer types and fall back to the is_finite() method. But even this can be too expensive.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 30, 2017

    While I don’t have much opinion either way, here is a patch to remove the existing dead code should that be the eventual outcome.

    If the default implementations in the base class deferred to math.isfinite() etc, that may help with compatibility.

    @vadmium vadmium added the 3.7 (EOL) end of life label Jan 30, 2017
    @mdickinson
    Copy link
    Member

    rm-finite.patch LGTM.

    Once that's merged, let's close this issue as rejected. If decimal is ever added as a first-class type (literal support, fixed-width decimal128 or decimal64, etc.), we may want to reconsider, but that day is probably a long way off.

    @rhettinger
    Copy link
    Contributor

    Mariatta, please review the rm-finite patch. We're already agreed that the removal makes sense. What you're checking for is whether patch is complete (that there aren't any dangling references to the removed code and that it doesn't remove too much). Also, try out the patch and run the test suite in in a debug mode.

    Whether you find any issues or are ready to bless the patch, make a comment in the tracker and assign back to Martin (the core dev who posts the patch is usually the one to apply it). After merging, he will mark the class as closed/rejected.

    @Mariatta
    Copy link
    Member

    Mariatta commented Feb 3, 2017

    Martin,

    I reviewed the patch (rm-finite.patch) and ran the tests. The patch no longer applies cleanly to the default branch, but otherwise looks good.

    Assigning this back to you as per Raymond's suggestion.

    Thanks :)

    @Mariatta Mariatta assigned vadmium and unassigned Mariatta Feb 3, 2017
    @corona10
    Copy link
    Member

    rm-finite.patch was applied by bpo-39415.

    Once that's merged, let's close this issue as rejected.

    I close this issue to a rejected state as discussed above.
    Thank you to all who participated in this discussion.

    @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.7 (EOL) end of life type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants