msg313912  (view) 
Author: Mark Dickinson (mark.dickinson) * 
Date: 20180315 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) * 
Date: 20180315 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 nonintegral floats. Maybe it was planned to extend factorial() to nonintegral 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) * 
Date: 20180316 08:15 
I'd suggest that in the notfloat branch of math_factorial, we use PyNumber_Index to catch integerlike things. (That would also be consistent with what we do in math_gcd.)

msg313942  (view) 
Author: Serhiy Storchaka (serhiy.storchaka) * 
Date: 20180316 08:55 
I agree. And I suggest also to deprecate accepting integral float instances.

msg314084  (view) 
Author: Mark Dickinson (mark.dickinson) * 
Date: 20180319 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) * 
Date: 20180319 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) * 
Date: 20180321 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 harmlessenough 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 godonlyknowswhatelse is just as obviously confused. So clean that up and declare victory ;)

msg322583  (view) 
Author: Tal Einat (taleinat) * 
Date: 20180728 18:05 
So we keep things consistent by supporting Decimal and Fraction inputs, but raising ValueError if the value isn't a nonnegative integer?

msg322625  (view) 
Author: Mark Dickinson (mark.dickinson) * 
Date: 20180729 09:40 
[Tal Einat]
> So we keep things consistent by supporting Decimal and Fraction inputs, but raising ValueError if the value isn't a nonnegative integer?
Rereading 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 integerlike (in the sense of implementing __index__), and complain with a TypeError if it isn't
 defer discussion of deprecating acceptance of integervalued 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) * 
Date: 20180729 11:08 
As a user, I would find Mark's suggestion very confusing, accepting nonintegral floats but not nonintegral Decimals and Fractions.
I agree that ideally it should accept only integral inputs, including floats, but deprecating the behavior for nonintegral 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 nonnegative integers.
At a later point we could deprecate this behavior for all noninteger 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) * 
Date: 20180730 07:30 
> accepting nonintegral floats but not nonintegral Decimals and Fractions.
I don't think anyone is proposing to accept nonintegral floats. nonintegral floats are _already_ rejected with a ValueError. Did you mean "integral" instead of "nonintegral"?

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

msg322663  (view) 
Author: Mark Dickinson (mark.dickinson) * 
Date: 20180730 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 nonintegral types, then it seems odd to deliberately add a new intentional feature (acceptance of integralvalued 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 integerlike 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) * 
Date: 20180730 11:15 
Understood. Thanks for making the time and effort to clarify!

msg322750  (view) 
Author: STINNER Victor (vstinner) * 
Date: 20180731 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) * 
Date: 20180731 19:58 
[Victor]
> Is it really important to accept integral float?
Possibly not, but acceptance of integral floats is deliberate, bydesign, 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) * 
Date: 20180731 20:52 
>> Is it really important to accept integral float?
> Possibly not, but acceptance of integral floats is deliberate, bydesign, 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) * 
Date: 20180903 21:20 
New changeset e9ba3705de656215d52b8f8f4a2e7ad60190e944 by Pablo Galindo in branch 'master':
bpo33083  Make math.factorial reject arguments that are not intlike (GH6149)
https://github.com/python/cpython/commit/e9ba3705de656215d52b8f8f4a2e7ad60190e944

msg324602  (view) 
Author: Mark Dickinson (mark.dickinson) * 
Date: 20180904 20:33 
The issue description mentions 3.7 and 2.7, but GH6149 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) * 
Date: 20180904 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) * 
Date: 20180907 18:44 
Since this is potentially breaking change, please add a What's New entry for it.

msg324793  (view) 
Author: STINNER Victor (vstinner) * 
Date: 20180907 21:09 
I reopen the issue since Serhiy requested a NEWS entry.

msg324812  (view) 
Author: Pablo Galindo Salgado (pablogsal) * 
Date: 20180907 23:16 
New changeset fa221d804f1bc07d992f820069bad24f176ed66d by Pablo Galindo in branch 'master':
bpo33083: Update "What's new" with math.factorial changes (GH9109)
https://github.com/python/cpython/commit/fa221d804f1bc07d992f820069bad24f176ed66d
