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

Expand math.gcd() and math.lcm() to accept multiple arguments #83829

Closed
ananthan-123 mannequin opened this issue Feb 16, 2020 · 27 comments
Closed

Expand math.gcd() and math.lcm() to accept multiple arguments #83829

ananthan-123 mannequin opened this issue Feb 16, 2020 · 27 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir

Comments

@ananthan-123
Copy link
Mannequin

ananthan-123 mannequin commented Feb 16, 2020

BPO 39648
Nosy @tim-one, @rhettinger, @mdickinson, @stevendaprano, @serhiy-storchaka, @sweeneyde, @ananthan-123, @bernardosulzbach
PRs
  • bpo-39648:[WIP]updated math.gcd to accept 'n' arguments. #18590
  • bpo-39648: Expand math.gcd() and math.lcm() to handle multiple arguments. #18604
  • 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 2020-02-23.11:23:14.671>
    created_at = <Date 2020-02-16.13:38:42.491>
    labels = ['library', '3.9']
    title = 'Expand math.gcd() and math.lcm() to accept multiple arguments'
    updated_at = <Date 2020-02-23.11:23:14.670>
    user = 'https://github.com/ananthan-123'

    bugs.python.org fields:

    activity = <Date 2020-02-23.11:23:14.670>
    actor = 'mark.dickinson'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-02-23.11:23:14.671>
    closer = 'mark.dickinson'
    components = ['Library (Lib)']
    creation = <Date 2020-02-16.13:38:42.491>
    creator = 'Ananthakrishnan'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39648
    keywords = ['patch']
    message_count = 27.0
    messages = ['362069', '362071', '362073', '362074', '362077', '362079', '362080', '362082', '362083', '362092', '362098', '362099', '362100', '362139', '362159', '362223', '362343', '362383', '362386', '362392', '362399', '362457', '362459', '362461', '362462', '362502', '362503']
    nosy_count = 9.0
    nosy_names = ['tim.peters', 'rhettinger', 'mark.dickinson', 'steven.daprano', 'SilentGhost', 'serhiy.storchaka', 'Dennis Sweeney', 'Ananthakrishnan', 'BernardoSulzbach']
    pr_nums = ['18590', '18604']
    priority = 'normal'
    resolution = None
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue39648'
    versions = ['Python 3.9']

    @ananthan-123
    Copy link
    Mannequin Author

    ananthan-123 mannequin commented Feb 16, 2020

    If we have to find the gcd of three or more numbers, now we should use
    gcd(a, gcd(b, gcd(c, gcd(d, e)))))
    which will create lot of problems.

    math.gcd should take "n" number of arguments,like:

    gcd(a,b,c,....)

    gcd(4,6,8) //returns 2
    gcd(2,5,8,6) //returns 1
    gcd(6,30,40,60,20,40) //returns 2

    @ananthan-123 ananthan-123 mannequin added 3.9 only security fixes stdlib Python modules in the Lib dir labels Feb 16, 2020
    @serhiy-storchaka
    Copy link
    Member

    What problems it will create?

    @ananthan-123
    Copy link
    Mannequin Author

    ananthan-123 mannequin commented Feb 16, 2020

    It will take more time to write the statement itself and there are chances of getting mistakes as the statement is complicated.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Feb 16, 2020

    You could (should?) use reduce, then:

    >>> functools.reduce(math.gcd, (6, 30, 40, 60, 20, 40))
    2

    @rhettinger
    Copy link
    Contributor

    +1 This isn't hard for us to do and may be occasionally useful.

    @serhiy-storchaka
    Copy link
    Member

    Should it take variable number of positional arguments or a single iterable argument as max() and min()?

    What should it return for 0 arguments?

    @serhiy-storchaka
    Copy link
    Member

    Python as programming language provides builtin blocks. This is a good example of using functools.reduce().

    But if you can provide an algorithm for calculating the GCD and LCM more efficient than sequential applying gcd() and lcm() for two arguments, it would be an argument for implementing it in the stdlib.

    @mdickinson
    Copy link
    Member

    Should it take variable number of positional arguments or a single iterable argument as max() and min()?

    Good question: the obvious extension to the current function would be to allow it to take multiple scalar arguments. But I don't much like the mismatch that introduces with sum and math.prod, which accept a single iterable argument.

    I definitely don't want to give gcd a combination API like the one min and max have.

    What should it return for 0 arguments?

    That one's easy. It should return 0. 0 is an identity for the binary gcd operation, and mathematically, the gcd is defined for any set of integers (finite or infinite), including the empty set. The gcd of the empty set is zero.

    @mdickinson
    Copy link
    Member

    What's slightly more interesting is the return value for a single argument a, where we should be careful to return abs(a) rather than simply a.

    @tim-one
    Copy link
    Member

    tim-one commented Feb 16, 2020

    This is almost all down to pragmatics for me.

    For sum() and prod(), if there are only two operands then there are trivial other ways to spell that (+ and *). So it makes most sense for them to accept iterables instead. Essentially, e.g., nobody would ever _want_ to write sum(2, 3) instead of 2+3.

    max() and min() differ in that there is no other simple way to write the 2-argument forms. max(x, y) _is_ commonly wanted - but so is max(iterable). It's a weird function signature, but nearly always does what's intended.

    gcd() (and lcm()!) differ from both in that they're nearly always wanted with 2 arguments. "0, 1, or >= 3 arguments" are possibilities, but rarely wanted. It would be a PITA to need to write, e.g., gcd((x, y)) instead of gcd(x, y) for the overwhelmingly most common 2-argument case. So they should be *args functions - provided we want to cater directly to "0, 1, or >= 3 arguments" at all. Fine by me if we don't. I'm only +0 on generalizing beyond "exactly 2 arguments".

    For the rest, I agree with Mark. In particular, gcd() must be 0.

    Serhiy, a "bulk" gcd can be more efficient than nested function calls by exploiting a common case: it can get out early when the gcd-so-far becomes 1 (since gcd(1, anything) == 1, it doesn't matter what the remaining arguments are). For "random" integers, it's already the case that gcd(i, j) == 1 over 60% of the time.

    @sweeneyde
    Copy link
    Member

    I think the behavior of gcd() == 0 is correct, but it should be documented, because it isn't completely obvious.

    Arguments for gcd() == 0:

    • Preserves the invariant gcd(itertools.chain(iterables)) == gcd(itertools.starmap(gcd, iterables)) in the case that some iterables are empty.

    • Return a "neutral element" (not technically true for negative integers because gcd(0, a) == abs(a) != a, but it does get "ignored")

    Other considerations:

    • The "common divisors" of the empty set are the integers d such that for all x in the empty set, d divides x, which is to say, all of the integers. Then there is no greatest of these common divisors. The same is true if we try gcd(0, 0, ... 0).

    • The above is only fixed if we interpret "greatest" in the sense of divisibility, where 0 > everything because everything divides 0. In particular, we can say that g is a greatest common divisor of a_1, ..., a_n if g | a_i for all i, and for any d with d | a_i for all i, we also have d | g. Using this definition also requires that we specify that if there are two gcds, we return the positive one.
      I believe this is the correct interpretation, but it may not be obvious, since it assumes that proof that such a number even exists.

    • Another common definition of the greatest common divisor of {a_1, ..., a_n} is the smallest positive integer expressible as an integer linear combination x_1 a_1 + ... + x_1 a_2, where x_i are integers. But this definition breaks on gcd(), gcd(0), gcd(0, 0), etc, since no positive integers are expressible.

    In short, while I agree with the behavior, I think there should be a clarifying sentence in the docs saying "returns 0 if all of the arguments are 0".

    @sweeneyde
    Copy link
    Member

    Correction: gcd(itertools.chain(iterables)) == gcd(*map(gcd, iterables))

    @sweeneyde
    Copy link
    Member

    Correction correction: returning zero preserves the invariant below.

    from math import gcd as GCD
    from functools import reduce
    from itertools import starmap, chain
    
    def gcd(*args):
        return reduce(GCD, args, 0)
    
    iterables = [[10, 20, 30],
                 [],
                 [0, 0, 0],
                 [5],
                 [-15, -10000000]]
    
    a = gcd(*chain.from_iterable(iterables))
    b = gcd(*starmap(gcd, iterables))
    assert a == b == 5

    @ananthan-123
    Copy link
    Mannequin Author

    ananthan-123 mannequin commented Feb 17, 2020

    It should take variable number of positional arguments.
    it should return:

    >math.gcd()
    0
    >math.gcd(8)
    8
    >math.gcd(-8)
    8

    @bernardosulzbach
    Copy link
    Mannequin

    bernardosulzbach mannequin commented Feb 17, 2020

    I think this might make sense because gcd() could have some optimizations for n > 2, such as sorting the numbers and starting by the smallest elements.

    However, I don't think gcd() and lcm() with more than two arguments are frequent use cases.

    @ananthan-123
    Copy link
    Mannequin Author

    ananthan-123 mannequin commented Feb 18, 2020

    Can I put together a PR for this issue?

    @mdickinson
    Copy link
    Member

    @ananthakrishnan: Sure, go ahead.

    Let's make the process a bit more streamlined than last time around: see if you can put together a PR that passes all the CI checks before asking for review. If you get stuck, make a work-in-progress PR and ask for help in a comment on that PR rather than via email.

    A couple of pointers:

    • Take a look at the existing math.hypot implementation to see how to deal with multiple arguments. (Note that argument clinic doesn't currently work for *args functions.)

    • For the first working version, don't bother making special cases for the zero-argument or one-argument gcd (or even the case of two arguments). You just want the equivalent of the following Python code, which is perfectly general:

        def gcd(*args):
            g = 0
            for arg in args:
                g = gcd_2arg(g, operator.index(arg))
            return g

    where gcd_2arg is the original 2-argument gcd (in C, _PyLong_GCD). We can always optimise later.

    @ananthan-123
    Copy link
    Mannequin Author

    ananthan-123 mannequin commented Feb 21, 2020

    This is my code for math.gcd:

    static PyObject *
    math_gcd(PyObject *module, PyObject *args, Py_ssize_t n)
    {
        PyObject *g = 0, *item, *in;
        
        Py_ssize_t i;
        for (i = 0; i < n; i++) {
            item = args[i];
            in = PyNumber_Index(item);
            if (in == NULL) {
                return NULL;
            }
            g = _PyLong_GCD(g, in);
        }
        Py_DECREF(in);
        return g;
    }

    This is showing an error:

    error: incompatible types when assigning to type ‘PyObject * {aka struct _object *}’ from type ‘PyObject {aka struct _object}’
    item = args[i];
    ^^^

    @mdickinson
    Copy link
    Member

    @ananthakrishnan: Do you know how arrays and pointers work in C?

    @ananthan-123
    Copy link
    Mannequin Author

    ananthan-123 mannequin commented Feb 21, 2020

    Yes I know.

    @ananthan-123
    Copy link
    Mannequin Author

    ananthan-123 mannequin commented Feb 21, 2020

    Thanks for the hint.Made changes.

    @serhiy-storchaka
    Copy link
    Member

    Sorry, Ananthakrishnan, but I think this problem is too difficult to you. Adding math.lcm() taken 2 weeks and produced 200 messages. It is simpler to implement this feature myself to me.

    @ananthan-123
    Copy link
    Mannequin Author

    ananthan-123 mannequin commented Feb 22, 2020

    >Sorry, Ananthakrishnan, but I think this problem is too difficult to you. Adding math.lcm() taken 2 weeks and produced 200 messages. It is simpler to implement this feature myself to me.

    I'm a beginner.Not everyone is perfect at begenning.
    I am starting this.So I took some problems myself as i saw the problems here are difficult.You should have given me a chance.

    I think you should have at the same situations once,like the situations that I'm facing now.

    @serhiy-storchaka
    Copy link
    Member

    I'm always ready to help a beginner, but this is the first time that it takes so much effort. It would be easier if you started with the simple problems you could handle. Even in simple cases you would be able to get tips that you can understand and gradually improve your skills.

    @serhiy-storchaka
    Copy link
    Member

    If expand math.gcd() to accept multiple arguments, it is worth to do the same with math.lcm().

    @serhiy-storchaka serhiy-storchaka changed the title Update math.gcd() to accept "n" arguments. Expand math.gcd() and math.lcm() to accept multiple arguments Feb 22, 2020
    @serhiy-storchaka serhiy-storchaka changed the title Update math.gcd() to accept "n" arguments. Expand math.gcd() and math.lcm() to accept multiple arguments Feb 22, 2020
    @mdickinson
    Copy link
    Member

    New changeset 559e7f1 by Serhiy Storchaka in branch 'master':
    bpo-39648: Expand math.gcd() and math.lcm() to handle multiple arguments. (GH-18604)
    559e7f1

    @mdickinson
    Copy link
    Member

    Merged; thank you to all involved.

    @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 stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants