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 a module for integer related math functions #81313

Closed
serhiy-storchaka opened this issue Jun 2, 2019 · 12 comments
Closed

Add a module for integer related math functions #81313

serhiy-storchaka opened this issue Jun 2, 2019 · 12 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 37132
Nosy @tim-one, @rhettinger, @mdickinson, @serhiy-storchaka
PRs
  • bpo-37132: Add the imath module. #13741
  • 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-06-03.07:13:59.683>
    created_at = <Date 2019-06-02.10:54:57.404>
    labels = ['3.8', 'type-feature', 'library']
    title = 'Add a module for integer related math functions'
    updated_at = <Date 2020-05-07.16:27:37.033>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2020-05-07.16:27:37.033>
    actor = 'mark.dickinson'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-06-03.07:13:59.683>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2019-06-02.10:54:57.404>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37132
    keywords = ['patch']
    message_count = 12.0
    messages = ['344269', '344274', '344277', '344279', '344280', '344281', '344283', '344296', '344297', '344381', '344387', '368354']
    nosy_count = 5.0
    nosy_names = ['tim.peters', 'rhettinger', 'mark.dickinson', 'stutzbach', 'serhiy.storchaka']
    pr_nums = ['13741']
    priority = 'normal'
    resolution = 'rejected'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue37132'
    versions = ['Python 3.8']

    @serhiy-storchaka
    Copy link
    Member Author

    The math module contains as function for floating-point numbers, as wells as functions for integer only numbers: factorial(), gcd(). Yet few integer specific functions was added in 3.8: isqrt(), perm(), comb().

    The proposed PR adds the new imath module, adds into it old functions factorial() and gcd() and moves new functions. It also adds two additional functions: as_integer_ratio() and ilog2().

    There are plans for adding more integer functions: divide_and_round(), is_prime(), primes(), but the work in progress.

    @serhiy-storchaka serhiy-storchaka added 3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jun 2, 2019
    @mdickinson
    Copy link
    Member

    Some questions:

    • What's the plan for the existing functions math.gcd and math.factorial? Do they eventually get deprecated, or do we keep gcd and factorial in both math and imath?

    • Does PEP-399 apply here? If so, we'll need a pure Python version as well as a C version

    • Should imath.isqrt be renamed to imath.sqrt?

    • What's the actual benefit of this separation?

    This feels like a really big change to be making a day before feature freeze; I'm not convinced that we don't need a PEP for this. How bad would it be if we have to defer until 3.9?

    @mdickinson
    Copy link
    Member

    Should imath.isqrt be renamed to imath.sqrt?

    On futher reflection, I don't think it should. It's a different function, so it's not like comparing math.sqrt and cmath.sqrt.

    One more: I suggest renaming the new function ilog to ilog2, for consistency with math.log versus math.log2.

    @serhiy-storchaka
    Copy link
    Member Author

    What's the plan for the existing functions math.gcd and math.factorial?

    I think they should eventually get deprecated, but with long term of deprecation. Deprecation warning should be added only when imath versions are already available in older Python versions.

    But they can be also kept as pure aliases for very long time.

    Does PEP-399 apply here?

    There were no Python implementations when these functions was in the math module. But there are Python implementations for all these functions (used in tests or as predecessors), so it is not hard to add them in the stdlib. Easier that to implement math.sin() on Python from scratch.

    Should imath.isqrt be renamed to imath.sqrt?

    I considered this idea and do not have answer.

    What's the actual benefit of this separation?

    Currently math becomes a mix of very different functions. Separation will help to keep it simpler (from user and implementation sides). It also adds a place for landing new integer specific functions which are too specific to add them into the math module or into the int class.

    @mdickinson
    Copy link
    Member

    I think the new as_integer_ratio also needs discussion: it was agreed at some point in the past to add as_integer_ratio methods on all numeric built-in types, and there's a PR for that. Adding as_integer_ratio as a new function too seems like two ways to do it.

    Please can we defer to 3.9? There just isn't time for the design discussions that are needed, and I'd personally rather see the first version of the imath module be somewhat complete, with is_prime and primes already.

    I do realise that from the perspective of adding an imath module, the accumulation of integer-based functions in the math module is something of a wart that we're making worse in 3.8, but I don't think rushing in this change is the solution.

    Radical suggestion: should we consider delaying the inclusions of comb, perm and isqrt in the math module so that we can do this properly for 3.9?

    @serhiy-storchaka
    Copy link
    Member Author

    Actually it was ilog2. There was just an error in the documentation.

    Currently we have two ways to represent the number as an integer ratio. The official way is two properties numerator and denominator, every number should have them. And some concrete numeric types have the as_integer_ratio() method which returns both values. Rather of adding as_integer_ratio() to all other numeric types I propose to add a helper function which uses either the as_integer_ratio() method if available, or the numerator and denominator properties. Currently any code that uses the as_integer_ratio() method should implement a fallback to numerator and denominator. With imath.as_integer_ratio() it can just use this function.

    Radical suggestion: should we consider delaying the inclusions of comb, perm and isqrt in the math module so that we can do this properly for 3.9?

    I like it. I suggest to delay also adding the int.as_integer_ratio() method. Currently it does not help because there are other numeric methods without the as_integer_ratio() method.

    @mdickinson
    Copy link
    Member

    I'm pretty much out of core-dev cycles for this weekend; I'm happy to review #57950, but won't have time to do so before next weekend.

    I'm overall -0 on this change; there's a minor benefit in the separation, but for me it's outweighted by the duplication of gcd and factorial (or in the case of gcd, triplication, since it's also in the fractions module).

    I'm out of time, so I'll be brief, but if this does go into 3.8, here are my thoughts:

    • please remove the imath.as_integer_ratio function; it needs more discussion, and it has quite a different character from the other functions; it's not at all clear to me that it belongs. We might end up putting it back in eventually, but it needs discussion first, and we don't have time for that discussion before feature freeze.
    • please add the pure Python version of imath that PEP-399 requires, or get Brett's confirmation that PEP-399 doesn't apply in this case.
    • please get agreement from one (preferably both) of Tim and Raymond

    Raymond, Tim: thoughts?

    @rhettinger
    Copy link
    Contributor

    -1 I prefer these functions get left in the regular math module and just organize the docs to separate out integer functions. Additional namespacing creates cognitive load and creates a findability problem.

    Also, I really don't want gcd() to be moved yet again.

    @rhettinger
    Copy link
    Contributor

    After the beta feature freeze, I'll write up an edit the math module docs that makes in easier to find functions (by splitting out a section of the docs for these functions and by creating a summary link table at the top like we do for builtins). I really don't want a separate module for this and think that would work against the needs of usres.

    @tim-one
    Copy link
    Member

    tim-one commented Jun 3, 2019

    Ya, I'm mostly with Raymond. math was originally a very thin wrapper around C's libm, but we've been moving away from that more and more for decades, and it's no longer the case anyway that the vast bulk of new Python programmers are intimately familiar with C.

    If I were coming new to Python now, I'd expect math functions to ... be in math ;-) Whether integer or float. I expect this will work fine if the docs are - as Raymond suggests - reworked to make it clear.

    @serhiy-storchaka
    Copy link
    Member Author

    Well, then closing this.

    @mdickinson
    Copy link
    Member

    See also bpo-40028

    @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.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants