math.factorial accepts non-integral Decimal instances

@tim-one

BPO	33083
Nosy	@tim-one, @rhettinger, @terryjreedy, @facundobatista, @mdickinson, @vstinner, @taleinat, @serhiy-storchaka, @pablogsal
PRs	bpo-33083 - math.factorial accepts non-integral Decimal instances #6149 bpo-33083: Add elimination of non-int-like parameters in math.factorial to "What's new" #9109

^{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 2018-09-22.17:33:46.082>
created_at = <Date 2018-03-15.20:38:33.024>
labels = ['type-bug', '3.8']
title = 'math.factorial accepts non-integral Decimal instances'
updated_at = <Date 2018-09-22.17:33:46.081>
user = 'https://github.com/mdickinson'

bugs.python.org fields:

activity = <Date 2018-09-22.17:33:46.081>
actor = 'taleinat'
assignee = 'none'
closed = True
closed_date = <Date 2018-09-22.17:33:46.082>
closer = 'taleinat'
components = []
creation = <Date 2018-03-15.20:38:33.024>
creator = 'mark.dickinson'
dependencies = []
files = []
hgrepos = []
issue_num = 33083
keywords = ['patch']
message_count = 23.0
messages = ['313912', '313917', '313936', '313942', '314084', '314085', '314179', '322583', '322625', '322628', '322657', '322658', '322663', '322669', '322750', '322813', '322818', '324537', '324602', '324604', '324783', '324793', '324812']
nosy_count = 10.0
nosy_names = ['tim.peters', 'rhettinger', 'terry.reedy', 'facundobatista', 'mark.dickinson', 'vstinner', 'taleinat', 'serhiy.storchaka', 'John.Yeung', 'pablogsal']
pr_nums = ['6149', '9109']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33083'
versions = ['Python 3.8']

Observed by Terry Reedy in the issue bpo-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)

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 bpo-2138. math.gamma() was added 1.5 years later in bpo-3366.

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.)

I agree. And I suggest also to deprecate accepting integral float instances.

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.

"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.

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 ;-)

So we keep things consistent by supporting Decimal and Fraction inputs, but raising ValueError if the value isn't a non-negative integer?

[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.

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.

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"?

> 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.

Understood. Thanks for making the time and effort to clarify!

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.

[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.

> 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.

New changeset e9ba370 by Pablo Galindo in branch 'master':
bpo-33083 - Make math.factorial reject arguments that are not int-like (GH-6149)
e9ba370

The issue description mentions 3.7 and 2.7, but #50399 wasn't marked for backport. My feeling is that it isn't worth backporting the fix, in which case this issue can be closed.

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.

Since this is potentially breaking change, please add a What's New entry for it.

I reopen the issue since Serhiy requested a NEWS entry.

New changeset fa221d8 by Pablo Galindo in branch 'master':
bpo-33083: Update "What's new" with math.factorial changes (GH-9109)
fa221d8

Navigation Menu

Repository navigation

math.factorial accepts non-integral Decimal instances #77264

1 remaining item

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

math.factorial accepts non-integral Decimal instances #77264

Description

Activity

mdickinson commented on Mar 15, 2018

serhiy-storchaka commented on Mar 15, 2018

mdickinson commented on Mar 16, 2018

serhiy-storchaka commented on Mar 16, 2018

mdickinson commented on Mar 19, 2018

serhiy-storchaka commented on Mar 19, 2018

tim-one commented on Mar 21, 2018

taleinat commented on Jul 28, 2018

mdickinson commented on Jul 29, 2018

taleinat commented on Jul 29, 2018

mdickinson commented on Jul 30, 2018

taleinat commented on Jul 30, 2018

1 remaining item

taleinat commented on Jul 30, 2018

vstinner commented on Jul 31, 2018

mdickinson commented on Jul 31, 2018

vstinner commented on Jul 31, 2018

pablogsal commented on Sep 3, 2018

mdickinson commented on Sep 4, 2018

vstinner commented on Sep 4, 2018

serhiy-storchaka commented on Sep 7, 2018

vstinner commented on Sep 7, 2018

pablogsal commented on Sep 7, 2018

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions