Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

math.factorial doc should mention integer return type #69921

Closed
JohnYeung mannequin opened this issue Nov 25, 2015 · 11 comments
Closed

math.factorial doc should mention integer return type #69921

JohnYeung mannequin opened this issue Nov 25, 2015 · 11 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir

Comments

@JohnYeung
Copy link
Mannequin

JohnYeung mannequin commented Nov 25, 2015

BPO 25735
Nosy @rhettinger, @terryjreedy, @facundobatista, @mdickinson, @skrah, @csabella, @miss-islington
PRs
  • bpo-25735: math.factorial doc should mention integer return type #6420
  • [3.7] bpo-25735: math.factorial doc should mention integer return type (GH-6420) #13702
    		•
    Files
  • bug.patch: edited math.rst
    		•

    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 2019-05-31.16:46:33.420>
created_at = <Date 2015-11-25.19:44:31.461>
labels = ['3.7', '3.8', 'docs']
title = 'math.factorial doc should mention integer return type'
updated_at = <Date 2019-05-31.16:58:30.519>
user = 'https://bugs.python.org/JohnYeung'

    bugs.python.org fields:

    activity = <Date 2019-05-31.16:58:30.519>
actor = 'miss-islington'
assignee = 'docs@python'
closed = True
closed_date = <Date 2019-05-31.16:46:33.420>
closer = 'cheryl.sabella'
components = ['Documentation']
creation = <Date 2015-11-25.19:44:31.461>
creator = 'John.Yeung'
dependencies = []
files = ['41206']
hgrepos = []
issue_num = 25735
keywords = ['patch']
message_count = 11.0
messages = ['255382', '255479', '255542', '255693', '313909', '313910', '313911', '313913', '344088', '344092', '344093']
nosy_count = 10.0
nosy_names = ['rhettinger', 'terry.reedy', 'facundobatista', 'mark.dickinson', 'skrah', 'docs@python', 'John.Yeung', 'mine0901', 'cheryl.sabella', 'miss-islington']
pr_nums = ['6420', '13702']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue25735'
versions = ['Python 3.7', 'Python 3.8']
    @JohnYeung
    Copy link
    Mannequin Author

    JohnYeung mannequin commented Nov 25, 2015

    The math module docs state

    Except when explicitly noted otherwise, all return values are floats.

    But math.factorial isn't what I would call explicit about returning int:

    math.factorial(x)
    Return x factorial. Raises ValueError if x is not integral or is negative.

    At minimum, shouldn't the first sentence be "Return x factorial as an int."? I haven't tested on all Python versions, but math.factorial on 2.7 and 3.2 definitely return int (or long in Python 2 when necessary).

    @JohnYeung JohnYeung mannequin assigned docspython Nov 25, 2015
    @JohnYeung JohnYeung mannequin added the docs Documentation in the Doc dir label Nov 25, 2015
    @terryjreedy
    Copy link
    Member 

    I agree.  In testing, I discovered this bug
>>> factorial(decimal.Decimal(5.2))
120
I don't know if this is a glitch in factorial or Decimal.

I also noticed
>>> fac(fractions.Fraction(4, 1))
Traceback (most recent call last):
  File "<pyshell#10>", line 1, in <module>
    fac(fractions.Fraction(4, 1))
TypeError: an integer is required (got type Fraction)
Perhaps this is due to no __int__ method.

    @mdickinson
    Copy link
    Member

    [Terry]

    >>> factorial(decimal.Decimal(5.2))
120

    Yep, that's definitely wrong. If we want to behave the same way as for float, we should accept only integral Decimal values. (Though I'm not much of a fan of the float behaviour: I would have preferred math.factorial not to accept floats at all.)

    @mine0901
    Copy link
    Mannequin

    mine0901 mannequin commented Dec 2, 2015

    States that math.factorial(x) returns x factorial as an integer.

    @csabella
    Copy link
    Contributor

    Should the documentation patch for this be converted to a PR or does this need to change to a bug issue to address Mark's concerns?

    Thanks!

    @csabella csabella added 3.7 (EOL) end of life 3.8 only security fixes labels Mar 15, 2018
    @mdickinson
    Copy link
    Member

    Should the documentation patch for this be converted to a PR

    I think so, yes. How about we open a new issue for the factorial(Decimal(...)) behaviour, and keep this one focused on the original reported issue?

    @csabella
    Copy link
    Contributor

    Sounds good. Thanks, Mark.

    @mine0901, would you like to open a Github pull request for your patch? Thanks!

    @mdickinson
    Copy link
    Member

    I opened bpo-33083, and copied the nosy list from this issue across.

    @csabella
    Copy link
    Contributor

    New changeset 4612671 by Cheryl Sabella (Akshay Sharma) in branch 'master':
    bpo-25735: math.factorial doc should mention integer return type (GH-6420)
    4612671

    @csabella
    Copy link
    Contributor

    @John.Yeung, thank you for the report. @mine0901, thanks for the initial patch and @akshaysharma, thank you for the PR.

    @miss-islington
    Copy link
    Contributor

    New changeset fc3b843 by Miss Islington (bot) in branch '3.7':
    bpo-25735: math.factorial doc should mention integer return type (GH-6420)
    fc3b843

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life 3.8 only security fixes docs Documentation in the Doc dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @mdickinson @terryjreedy @csabella @miss-islington