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.

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.

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