classification
Title: math.factorial accepts non-integral Decimal instances
Type: behavior Stage: resolved
Components: Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: John.Yeung, facundobatista, mark.dickinson, pablogsal, rhettinger, serhiy.storchaka, taleinat, terry.reedy, tim.peters, vstinner
Priority: normal Keywords: patch

Created on 2018-03-15 20:38 by mark.dickinson, last changed 2018-09-22 17:33 by taleinat. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 6149 merged pablogsal, 2018-03-19 00:57
PR 9109 merged pablogsal, 2018-09-07 22:01
Messages (23)
msg313912 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-03-15 20:38
Observed by Terry Reedy in the issue #25735 discussion (msg255479):

>>> factorial(decimal.Decimal(5.2))
120

This should be either raising an exception (either ValueError or TypeError, depending on whether we want to accept only integral Decimal values, or prohibit Decimal values altogether), or possibly returning an approximation to Gamma(6.2) (=169.406099461722999629...)

I'd prefer that we prohibit a Decimal input altogether, but accepting integral Decimal instances would parallel the current behaviour with floats:

>>> factorial(5.2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: factorial() only accepts integral values
>>> factorial(5.0)
120


Terry also observed:

>>> factorial(Fraction(5))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: an integer is required (got type Fraction)
msg313917 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-15 23:00
Many functions implemented in C accept Decimal instances.

>>> chr(decimal.Decimal(65.2))
'A'

This is because PyLong_AsLong() and similar functions call __int__().

Floats are specially prohibited when convert arguments with PyArg_Parse() with the "i" format unit.

>>> chr(65.2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: integer argument expected, got float

The specific of factorial() is that it accepts integral floats and raises ValueError instead of TypeError for non-integral floats. Maybe it was planned to extend factorial() to non-integral floats by using a gamma function. All other functions in the math module worked with floats at the time of adding factorial() in issue2138. math.gamma() was added 1.5 years later in issue3366.
msg313936 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-03-16 08:15
I'd suggest that in the not-float branch of math_factorial, we use PyNumber_Index to catch integer-like things. (That would also be consistent with what we do in math_gcd.)
msg313942 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-16 08:55
I agree. And I suggest also to deprecate accepting integral float instances.
msg314084 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-03-19 08:39
Raymond: what are your thoughts on deprecating the ability of `math.factorial` to accept a float (as in `math.factorial(5.0)` -> `120`)?

For me, I'm not sure I see the value of the deprecation. It's the usual story: the answer to "Knowing what we know now, should we have done this differently in the first place" is "Probably, yes". But "Should we change the current behaviour" is a very different question. I'm tempted to see the acceptance of integral floats as a harmless quirk, and the deprecation of that behaviour as a case of gratuitous breakage.
msg314085 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-19 08:52
"Special cases aren't special enough to break the rules."

Supporting floats is a special case. After ending the period of deprecation the code can be simplified.
msg314179 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2018-03-21 01:31
factorial(float) was obviously intended to work the way it does, so I'd leave it alone in whatever changes are made to resolve _this_ issue.  I view it as a harmless-enough quirk, but, regardless, if people want to deprecate it that should be a different issue.

What the code ends up doing for Decimal and Fraction and god-only-knows-what-else is just as obviously confused.  So clean that up and declare victory ;-)
msg322583 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-07-28 18:05
So we keep things consistent by supporting Decimal and Fraction inputs, but raising ValueError if the value isn't a non-negative integer?
msg322625 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-07-29 09:40
[Tal Einat]
> So we keep things consistent by supporting Decimal and Fraction inputs, but raising ValueError if the value isn't a non-negative integer?

Re-reading the issue comments, my preference is still the same as expressed in msg313936:

- no behaviour change if the input is a float
- if the input is not a float, require it to be integer-like (in the sense of implementing __index__), and complain with a TypeError if it isn't
- defer discussion of deprecating acceptance of integer-valued floats to another issue

So `math.factorial(x)` would become a `TypeError` for a `Decimal` or `Fraction` instance, whether integral or not.
msg322628 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-07-29 11:08
As a user, I would find Mark's suggestion very confusing, accepting non-integral floats but not non-integral Decimals and Fractions.

I agree that ideally it should accept only integral inputs, including floats, but deprecating the behavior for non-integral floats has been deemed out of scope for this issue.  Therefore I suggest doing the same for Decimal and Fraction: accept them but raise a ValueError if their values aren't non-negative integers.

At a later point we could deprecate this behavior for all non-integer types of numbers.

Another alternative, which is also more acceptable IMO, is to raise TypeError if given anything other than an int or a float, while deprecating floats altogether.
msg322657 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-07-30 07:30
> accepting non-integral floats but not non-integral Decimals and Fractions.

I don't think anyone is proposing to accept non-integral floats. non-integral floats are _already_ rejected with a ValueError. Did you mean "integral" instead of "non-integral"?
msg322658 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-07-30 07:55
>> accepting non-integral floats but not non-integral Decimals and Fractions.

> I don't think anyone is proposing to accept non-integral floats. non-integral floats are _already_ rejected with a ValueError. Did you mean "integral" instead of "non-integral"?

Indeed, yes.
msg322663 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-07-30 09:54
So I'm against _adding_ support for Decimal and Fraction on various grounds:

(1) If it's our intention to deprecate acceptance of non-integral types, then it seems odd to deliberately add a new intentional feature (acceptance of integral-valued Decimal objects) that we know we want to deprecate long term. It's only if we don't intend any such deprecation that it would make sense to add those new features.

(2) Implementation: adding support for Decimal objects in a math module is going to be messy, adding significant complication to the C code. The math module would have to import Decimal (something it doesn't currently need to do) in order to make the Decimal instance checks. @taleinat: how would you see the implementation working?

(3) If we support Fraction, should we also support arbitrary numbers.Rational instances? What's the exact rule for what should and shouldn't be accepted. The line isn't clear. In contrast, if the rule is that only floats and integer-like things are accepted, it's much clearer.

I think there's a clear goal here, which is that `math.factorial` should accept only integral types, defined as those implementing `__index__`. This is the same thing that `math.gcd` does.

To avoid gratuitous breakage, and because acceptance of floats was a deliberate design decision, we should continue to accept integral floats in the short term, perhaps eventually deprecating. But deliberately supporting integral Decimals and Fractions takes us further from the goal.
msg322669 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2018-07-30 11:15
Understood. Thanks for making the time and effort to clarify!
msg322750 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-31 09:59
I looked at Pablo's PR 6149 and I'm surprised by the size of the code just to convert a Python object to a C long!
---
    if (PyFloat_Check(arg)) {
        PyObject *lx;
        double dx = PyFloat_AS_DOUBLE((PyFloatObject *)arg);
        if (!(Py_IS_FINITE(dx) && dx == floor(dx))) {
            PyErr_SetString(PyExc_ValueError,
                            "factorial() only accepts integral values");
            return NULL;
        }
        lx = PyLong_FromDouble(dx);
        if (lx == NULL)
            return NULL;
        x = PyLong_AsLongAndOverflow(lx, &overflow);
        Py_DECREF(lx);
    }
    else {
        pyint_form = PyNumber_Index(arg);
        if (pyint_form == NULL) {
            return NULL;
        }
        x = PyLong_AsLongAndOverflow(pyint_form, &overflow);
        Py_DECREF(pyint_form);
    }

    if (x == -1 && PyErr_Occurred()) {
        return NULL;
    }
    else if (overflow == 1) {
        PyErr_Format(PyExc_OverflowError,
                     "factorial() argument should not exceed %ld",
                     LONG_MAX);
        return NULL;
    }
    else if (overflow == -1 || x < 0) {
        PyErr_SetString(PyExc_ValueError,
                        "factorial() not defined for negative values");
        return NULL;
    }
---
Do we really need 37 lines of C code? Is it really important to accept integral float? What's wrong with factorial(int(d)) for example?

PR 6149 LGTM, but since I was not involved in the discussion, I would prefer if someone else who has been involved would approve the change: Tal already approved it, but I saw some complains, I'm not 100% sure that we reached a full agreement.

Note: Pablo is a core developer and so he can merge his PR, I'm only asking to *approve* the change, not to merge it ;-)

Thanks in advance.

Note2: I'm still mentoring Pablo after he became a core, and I require him to ask me before merging any change.
msg322813 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-07-31 19:58
[Victor]

> Is it really important to accept integral float?

Possibly not, but acceptance of integral floats is deliberate, by-design, tested behaviour, so it at least deserves a deprecation period if we're going to get rid of it. (I'm -0.0 on such a deprecation - it doesn't seem worth the code churn and the possible breakage.)

If we want to consider deprecation, please let's open a new issue for that. Then this one can be closed when Pablo's PR is merged.

Pablo's changes LGTM; I've added my approval to the PR.
msg322818 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-31 20:52
>> Is it really important to accept integral float?

> Possibly not, but acceptance of integral floats is deliberate, by-design, tested behaviour, so it at least deserves a deprecation period if we're going to get rid of it.

Ok. Let's keep it in that case ;-)

I also approved Pablo's PR.
msg324537 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2018-09-03 21:20
New changeset e9ba3705de656215d52b8f8f4a2e7ad60190e944 by Pablo Galindo in branch 'master':
bpo-33083 - Make math.factorial reject arguments that are not int-like (GH-6149)
https://github.com/python/cpython/commit/e9ba3705de656215d52b8f8f4a2e7ad60190e944
msg324602 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2018-09-04 20:33
The issue description mentions 3.7 and 2.7, but GH-6149 wasn't marked for backport. My feeling is that it isn't worth backporting the fix, in which case this issue can be closed.
msg324604 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-04 21:33
I agree to not backport the change to 3.7 and older. The change can be seen as subtle behaviour change which breaks backward compatibility.
msg324783 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-07 18:44
Since this is potentially breaking change, please add a What's New entry for it.
msg324793 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-07 21:09
I reopen the issue since Serhiy requested a NEWS entry.
msg324812 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2018-09-07 23:16
New changeset fa221d804f1bc07d992f820069bad24f176ed66d by Pablo Galindo in branch 'master':
bpo-33083: Update "What's new" with math.factorial changes (GH-9109)
https://github.com/python/cpython/commit/fa221d804f1bc07d992f820069bad24f176ed66d
History
Date User Action Args
2018-09-22 17:33:46taleinatsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-09-07 23:16:20pablogsalsetmessages: + msg324812
2018-09-07 22:01:02pablogsalsetstage: resolved -> patch review
pull_requests: + pull_request8563
2018-09-07 21:09:50vstinnersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg324793
2018-09-07 18:44:08serhiy.storchakasetmessages: + msg324783
2018-09-04 21:33:33vstinnersetstatus: pending -> closed

messages: + msg324604
versions: - Python 2.7, Python 3.7
2018-09-04 20:34:25mark.dickinsonsetstatus: open -> pending
resolution: fixed
stage: patch review -> resolved
2018-09-04 20:33:35mark.dickinsonsetmessages: + msg324602
2018-09-03 21:20:08pablogsalsetnosy: + pablogsal
messages: + msg324537
2018-07-31 20:52:32vstinnersetmessages: + msg322818
2018-07-31 19:58:33mark.dickinsonsetmessages: + msg322813
2018-07-31 09:59:08vstinnersetnosy: + vstinner
messages: + msg322750
2018-07-30 11:15:48taleinatsetmessages: + msg322669
2018-07-30 09:54:55mark.dickinsonsetmessages: + msg322663
2018-07-30 07:55:48taleinatsetmessages: + msg322658
2018-07-30 07:30:02mark.dickinsonsetmessages: + msg322657
2018-07-29 11:08:59taleinatsetmessages: + msg322628
2018-07-29 09:40:40mark.dickinsonsetmessages: + msg322625
2018-07-28 18:05:17taleinatsetnosy: + taleinat
messages: + msg322583
2018-03-21 01:31:47tim.peterssetnosy: + tim.peters
messages: + msg314179
2018-03-19 08:52:15serhiy.storchakasetmessages: + msg314085
2018-03-19 08:39:04mark.dickinsonsetmessages: + msg314084
2018-03-19 00:57:09pablogsalsetkeywords: + patch
stage: patch review
pull_requests: + pull_request5907
2018-03-16 08:55:49serhiy.storchakasetmessages: + msg313942
2018-03-16 08:15:05mark.dickinsonsetmessages: + msg313936
2018-03-15 23:26:21skrahsetnosy: - skrah
2018-03-15 23:00:55serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg313917
2018-03-15 21:14:25John.Yeungsetnosy: + John.Yeung
2018-03-15 20:38:33mark.dickinsoncreate