-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Add as_integer_ratio() to int() objects #77254
Comments
Goal: make int() more interoperable with float by making a float/Decimal method also available on ints. This will let mypy treat ints as a subtype of floats. See: https://mail.python.org/pipermail/python-dev/2018-March/152384.html Open question: Is this also desired for fractions.Fraction and numbers.Rational? |
I think so. The idea, e.g., that "it's obvious" for Fraction is no more compelling than that it's even more obvious for ints ;-) Given that it's spreading to ints anyway, there's an opportunity to make it possible for clients to write utterly uniform code for every type whose values can be represented as integer ratios. I'd also say it's desirable to extend the Fraction constructor, to accept argument(s) of any type(s) that implement as_integer_ratio(). But that should probably be a different issue. |
On the Fraction constructor, it looks like there's some overlap with the goals of issue bpo-28716 here. |
Okay, let's focus this issue on just int.as_integer_ratio() so that Nofar can have something small and self contained to work on :-) |
I'm working on it |
Wouldn't be better to add the function as_integer_ration() in the math module (or in more appropriate place)? def as_integer_ration(x):
if hasattr(x, 'as_integer_ration'):
return x.as_integer_ration()
else:
return (x.numerator, x.denominator) The advantage over adding the int method is that it will automatically support other rational numbers like NumPy integers. |
Serhiy, we already went down the path of implementing this as a method. Of course |
Actually numbers.Rational is a virtual base class for int, so it won't automagically appear there. Adding it to the math module is inferior because for non-rational types (e.g. alternative float implementations) the math module won't have the knowledge about internals to implement it -- and casting to float() would defeat the purpose. |
Thanks, Guido! I figured I was missing something :-) It looks like |
+1. On Tue, Mar 13, 2018, 16:26 Tim Peters <report@bugs.python.org> wrote:
|
Adding as_integer_ratio() to numbers.Rational is a breaking change. It breaks the interface. Currently all numbers.Rational subclasses implement all public methods and properties of the abstract class, but with adding as_integer_ratio() this will be no longer true. >>> issubclass(numpy.int64, numbers.Rational)
True
>>> numpy.int64.as_integer_ratio
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: type object 'numpy.int64' has no attribute 'as_integer_ratio' I suggest to make as_integer_ratio() creating an implicit interface (as write() creates an implicit interface for writable file-like objects), and add a convenient function (in the numbers or the math module) which calls this method if it exists and falls back to the (nominator, denominator) pair otherwise. |
Serhiy, I don't understand. If class Rational(Real):
...
def as_integer_ratio(self):
return (self.numerator, self.denominator) Or, as for Python ints, is Rational a "make believe" (virtual) superclass of numpy.int64? |
It's a "virtual" subclass. The numpy.integer parent class is registered here: |
Thanks, Mark! So if int.as_integer_ratio is added to the core, numpy.int64 won't magically have it too, regardless of whether we do or don't add an implementation to numbers.Rational. As an end user, I'd be surprised if numpy.int64 didn't support the same stuff as core ints. I doubt I'd care at all what numbers.Rational said in cases where it didn't. But rather than argue about that, what would _you_ like to see done here? |
[Tim]
Given that float.as_integer_ratio and Decimal.as_integer_ratio already exist, and that I don't have a working time machine right now, I'm in favour of adding int.as_integer_ratio purely for duck-typing reasons: I want to be able to use an int where a float is expected, and I _have_ encountered code in the past where I have to think too hard about what precise types are being used, and where that thinking seems as though it shouldn't be necessary. For the same reason, I'd support as_integer_ratio on the Fraction type. I don't really care whether it gets there via the Rational ABC or not. Adding as_integer_ratio to Rational as an @AbstractMethod _does_ seem a step too far (because now anyone who registers a type with numbers.Integral or numbers.Rational has to implement as_integer_ratio), but my understanding is that that's not what's being proposed. It doesn't bother me that the NumPy integer types won't support as_integer_ratio immediately (or possibly ever). The floating-point types don't either (except np.float64, which only has it because it inherits directly from Python's float), and it's hard to see how as_integer_ratio could be useful for a NumPy float32 array (for example), given that the returned numerator and denominator could exceed the bounds of any of the fixed-width NumPy integer types. I write a lot of NumPy-using code, but the world in which I do so rarely meets the world where I care about correct rounding, fractions, counting ulps error, etc. I haven't missed float32.as_integer_ratio or float16.as_integer_ratio yet. If we're worried about NumPy compatibility, it might be interesting to get Nathaniel Smith's opinion. |
Eric Wieser (added to CC) actually just opened a PR for this against NumPy: numpy/numpy#10741 I have weak and mixed feelings about the whole thing: numpy/numpy#10741 (comment) |
Sorry, I misspoke -- I meant he opened a related PR. The PR is to add as_integer_ratio to np.float16, np.float32, np.longdouble, not to add it to the numpy integer types. There are similar issues though. |
I remove the "easy (C)" keyword since the feature seems to be controversal (at least, not so easy). |
I assume it's decided what to do -- it may not be easy to do it, but I wouldn't call it controversial at this point. |
Ok, I delete "controversal" from my comment :-) To tag an issue as "easy" (or "easy C"), please describe step by step how you would implement the feature: modified files, modified functions, etc. It's ok to not give these explanation, but in that case, it's a regular issue, not an easy one. Note: I'm triagging old "easy" issues. I remove the keyword when I consider that the issue is not easy enough for a newcomer. |
I put the easy tag here because it was suitable for our mentees and because it is isn't a very difficult task (literally, this is the definition of "easy"). FWIW, it is not "triaging" when the tag was set by a seasoned core developer who was familiar with the issue and already working with a mentee. Triaging is making an initial assessment when no one else has looked at an issue. |
Nofar, do you want to continue to work on this or should I reassign? |
The merged PR contains several errors. See comments on GitHub. |
Some comments were seen after the merge of 8750 and were addressed in 9297. If you want to change the checked in code, please open a separate PR and tracker item. If you want different doc markup, please just fix it. This issue has already sprawled out of proportion to its usefulness and has eaten many hours of our time. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: