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

Optimize converting float and Decimal to Fraction #70159

Closed
serhiy-storchaka opened this issue Dec 28, 2015 · 12 comments
Closed

Optimize converting float and Decimal to Fraction #70159

serhiy-storchaka opened this issue Dec 28, 2015 · 12 comments
Assignees
Labels
interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Dec 28, 2015

BPO 25971
Nosy @rhettinger, @facundobatista, @mdickinson, @bitdancer, @skrah, @serhiy-storchaka
Files
  • fraction_from_floating.patch
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2015-12-29.21:10:08.621>
    created_at = <Date 2015-12-28.22:48:17.914>
    labels = ['interpreter-core', 'type-feature', 'library']
    title = 'Optimize converting float and Decimal to Fraction'
    updated_at = <Date 2015-12-29.21:10:08.620>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2015-12-29.21:10:08.620>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-12-29.21:10:08.621>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core', 'Library (Lib)']
    creation = <Date 2015-12-28.22:48:17.914>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = ['41442']
    hgrepos = []
    issue_num = 25971
    keywords = ['patch']
    message_count = 12.0
    messages = ['257145', '257201', '257205', '257206', '257207', '257208', '257209', '257211', '257212', '257213', '257215', '257216']
    nosy_count = 7.0
    nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'r.david.murray', 'skrah', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25971'
    versions = ['Python 3.6']

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Dec 28, 2015

    Proposed patch makes following things:

    1. Rewrite error messages in float.as_integer_ratio() and Python implementation of Decimal.as_integer_ratio() in more general form, not mentioning as_integer_ratio(), as in C implementation of Decimal.as_integer_ratio().

    2. Use Decimal.as_integer_ratio() to convert Decimal to Fraction.

    3. Get rid of additional checks in Fraction constructor that raise errors with appropriate messages, since new error messages from as_integer_ratio() are appropriate enough.

    This speeds up creating a Fraction from float and Decimal 2 to 3 times.

    @serhiy-storchaka serhiy-storchaka added interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Dec 28, 2015
    @mdickinson
    Copy link
    Member

    mdickinson commented Dec 29, 2015

    Any particular reason for the lower-casing of "Cannot" to "cannot" in the exception messages?

    Otherwise, LGTM.

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Dec 29, 2015

    Any particular reason for the lower-casing of "Cannot" to "cannot" in the exception messages?

    Only for matching current messages in C implementation of Decimal.as_integer_ratio(). As well as non-distinguishing positive and negative infinities, NaN and sNaN. If this is desirable, exception messages in C implementation of Decimal.as_integer_ratio() should be changed too.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Dec 29, 2015

    Both versions are fine with me. The lowercase "cannot do ..." is more
    pervasive in the source tree:

    $ grep -R '"cannot' | wc -l
    293
    $ grep -R '"Cannot' | wc -l
    150

    If we change it, let's change all occurrences in _pydecimal and
    _decimal/docstrings.h to uppercase (e.g. in __floor__, __round__,
    __ceil__).

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Dec 29, 2015

    From my non-native speaker's point of view, I'd use lowercase if
    the sentence doesn't end with a full stop, uppercase otherwise.

    When I started here I was told that error messages in Python
    usually don't have a full stop. ;)

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Dec 29, 2015

    Yes, this already was discussed in bpo-22364. The lowercase form wins with the score 4:1 (see msg226790, error messages use not only double quotes).

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Dec 29, 2015

    The question is wherever we should distinguish different sorts of infinities and NaNs (I think this is not needed).

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Dec 29, 2015

    Sorry for the bikeshedding, but I find this interesting:

    Poly/ML (Cambridge), ghci (Glasgow) and OCaml (INRIA) appear to
    use error messages without a full stop but starting in uppercase.

    SML/NJ (Bell Labs) uses lowercase (and no full stop).

    Perhaps this is a British/European vs. American issue?

    Regarding int/-inf: I don't think it's important to distiguish
    between them.

    @bitdancer
    Copy link
    Member

    bitdancer commented Dec 29, 2015

    I wonder if it is a (programming) language specific thing. On the other hand, I don't know what those langauges error messages look like, but Python's normally follow a colon (ValueError: ....) where not having the message capitalized is more natural (whether or not there is a full stop at the end), at least in American English.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Dec 29, 2015

    Serhiy: to me, the patch also looks good, we can certainly change
    any error messages later.

    Back to the bikeshedding:

    Poly/ML:
    ========

    f;
    Error-Value or constructor (f) has not been declared Found near f
    Static errors (pass2)

    ghci
    ====
    Prelude> f

    <interactive>:2:1: Not in scope: `f'

    OCaml
    =====

    # f;;
    Error: Unbound value f

    SML/NJ
    ======

    • f;
      stdIn:1.2 Error: unbound variable or constructor: f

    Basically the European languages start in uppercase after the
    first relevant colon (or hyphen in the case of Poly/ML).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 29, 2015

    New changeset 284026a8af9e by Serhiy Storchaka in branch 'default':
    Issue bpo-25971: Optimized creating Fractions from floats by 2 times and from
    https://hg.python.org/cpython/rev/284026a8af9e

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Dec 29, 2015

    Thank you all for your review.

    @serhiy-storchaka serhiy-storchaka self-assigned this Dec 29, 2015
    @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
    Labels
    interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants