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

Change by Tal Einat : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker ___ ___ Pyt

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

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

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

Change by Pablo Galindo Salgado : -- pull_requests: +8563 stage: resolved -> patch review ___ Python tracker ___ ___ Python-bugs-lis

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

STINNER Victor added the comment: I reopen the issue since Serhiy requested a NEWS entry. -- resolution: fixed -> status: closed -> open ___ Python tracker ___ __

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

Serhiy Storchaka added the comment: Since this is potentially breaking change, please add a What's New entry for it. -- ___ Python tracker ___

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

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

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

Change by Mark Dickinson : -- resolution: -> fixed stage: patch review -> resolved status: open -> pending ___ Python tracker ___ _

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

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

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

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/e9ba3705de656215d52b8f8f4a2e7ad60190e9

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

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 cas

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

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 depreca

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

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(d

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

Tal Einat added the comment: Understood. Thanks for making the time and effort to clarify! -- ___ Python tracker ___ ___ Python-bug

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

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 objec

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

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

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

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

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

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

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

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 behaviou

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

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 _

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

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.

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

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

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

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, sh

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

Change by Pablo Galindo Salgado : -- keywords: +patch pull_requests: +5907 stage: -> patch review ___ Python tracker ___ ___ Python-

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

Serhiy Storchaka added the comment: I agree. And I suggest also to deprecate accepting integral float instances. -- ___ Python tracker ___ _

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

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

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

Change by Stefan Krah : -- nosy: -skrah ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.o

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

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

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

Change by John Yeung : -- nosy: +John.Yeung ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.pytho

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

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