[issue33083] math.factorial accepts non-integral Decimal instances

[Tal Einat]

Change by Tal Einat :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Pablo Galindo Salgado]

Pablo Galindo Salgado  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Pablo Galindo Salgado]

Change by Pablo Galindo Salgado :


--
pull_requests: +8563
stage: resolved -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[STINNER Victor]

STINNER Victor  added the comment:

I reopen the issue since Serhiy requested a NEWS entry.

--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[STINNER Victor]

STINNER Victor  added the comment:

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.

--
status: pending -> closed
versions:  -Python 2.7, Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Mark Dickinson]

Change by Mark Dickinson :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> pending

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Mark Dickinson]

Mark Dickinson  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Pablo Galindo Salgado]

Pablo Galindo Salgado  added the comment:


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


--
nosy: +pablogsal

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[STINNER Victor]

STINNER Victor  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Mark Dickinson]

Mark Dickinson  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[STINNER Victor]

STINNER Victor  added the comment:

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, );
Py_DECREF(lx);
}
else {
pyint_form = PyNumber_Index(arg);
if (pyint_form == NULL) {
return NULL;
}
x = PyLong_AsLongAndOverflow(pyint_form, );
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.

--
nosy: +vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Tal Einat]

Tal Einat  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Mark Dickinson]

Mark Dickinson  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Tal Einat]

Tal Einat  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Mark Dickinson]

Mark Dickinson  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Tal Einat]

Tal Einat  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Mark Dickinson]

Mark Dickinson  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Tal Einat]

Tal Einat  added the comment:

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

--
nosy: +taleinat

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Tim Peters]

Tim Peters  added the comment:

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

--
nosy: +tim.peters

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Mark Dickinson]

Mark Dickinson  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Pablo Galindo Salgado]

Change by Pablo Galindo Salgado :


--
keywords: +patch
pull_requests: +5907
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Mark Dickinson]

Mark Dickinson  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Stefan Krah]

Change by Stefan Krah :


--
nosy:  -skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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 "", line 1, in 
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.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[John Yeung]

Change by John Yeung :


--
nosy: +John.Yeung

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue33083] math.factorial accepts non-integral Decimal instances

[Mark Dickinson]

New submission from Mark Dickinson :

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 "", line 1, in 
ValueError: factorial() only accepts integral values
>>> factorial(5.0)
120


Terry also observed:

>>> factorial(Fraction(5))
Traceback (most recent call last):
  File "", line 1, in 
TypeError: an integer is required (got type Fraction)

--
messages: 313912
nosy: facundobatista, mark.dickinson, rhettinger, skrah, terry.reedy
priority: normal
severity: normal
status: open
title: math.factorial accepts non-integral Decimal instances
type: behavior
versions: Python 2.7, Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com