classification
Title: Optimize converting float and Decimal to Fraction
Type: enhancement Stage: resolved
Components: Interpreter Core, Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: facundobatista, mark.dickinson, python-dev, r.david.murray, rhettinger, serhiy.storchaka, skrah
Priority: normal Keywords: patch

Created on 2015-12-28 22:48 by serhiy.storchaka, last changed 2015-12-29 21:10 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
fraction_from_floating.patch serhiy.storchaka, 2015-12-28 22:48 review
Messages (12)
msg257145 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-28 22:48
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.
msg257201 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2015-12-29 17:46
Any particular reason for the lower-casing of "Cannot" to "cannot" in the exception messages?

Otherwise, LGTM.
msg257205 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-29 18:47
> 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.
msg257206 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-12-29 18:57
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__).
msg257207 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-12-29 19:13
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. ;)
msg257208 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-29 19:14
Yes, this already was discussed in issue22364. The lowercase form wins with the score 4:1 (see msg226790, error messages use not only double quotes).
msg257209 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-29 19:17
The question is wherever we should distinguish different sorts of infinities and NaNs (I think this is not needed).
msg257211 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-12-29 19:47
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.
msg257212 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-12-29 19:55
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.
msg257213 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-12-29 20:16
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).
msg257215 - (view) Author: Roundup Robot (python-dev) Date: 2015-12-29 20:34
New changeset 284026a8af9e by Serhiy Storchaka in branch 'default':
Issue #25971: Optimized creating Fractions from floats by 2 times and from
https://hg.python.org/cpython/rev/284026a8af9e
msg257216 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-12-29 21:10
Thank you all for your review.
History
Date User Action Args
2015-12-29 21:10:08serhiy.storchakasetstatus: open -> closed
messages: + msg257216

assignee: serhiy.storchaka
resolution: fixed
stage: patch review -> resolved
2015-12-29 20:34:47python-devsetnosy: + python-dev
messages: + msg257215
2015-12-29 20:16:05skrahsetmessages: + msg257213
2015-12-29 19:55:38r.david.murraysetnosy: + r.david.murray
messages: + msg257212
2015-12-29 19:47:04skrahsetmessages: + msg257211
2015-12-29 19:17:42serhiy.storchakasetmessages: + msg257209
2015-12-29 19:14:32serhiy.storchakasetmessages: + msg257208
2015-12-29 19:13:38skrahsetmessages: + msg257207
2015-12-29 18:57:29skrahsetmessages: + msg257206
2015-12-29 18:47:19serhiy.storchakasetmessages: + msg257205
2015-12-29 17:46:30mark.dickinsonsetmessages: + msg257201
2015-12-28 22:48:17serhiy.storchakacreate