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

Add as_integer_ratio() to int() objects #77254

Closed
rhettinger opened this issue Mar 13, 2018 · 26 comments
Closed

Add as_integer_ratio() to int() objects #77254

rhettinger opened this issue Mar 13, 2018 · 26 comments
Assignees
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@rhettinger
Copy link
Contributor

BPO 33073
Nosy @tim-one, @rhettinger, @mdickinson, @vstinner, @njsmith, @serhiy-storchaka, @eric-wieser, @Shredder13, @lisroach
PRs
  • bpo-33073: Adding as_integer_ratio to ints. #8750
  • bpo-33073: Fix-up parenthesis, organization, and NULL check #9297
  • bpo-33073: Fix compiler warning with a type cast #9300
  • Post-commit updates for bpo-33073. #9303
  • 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/lisroach'
    closed_at = <Date 2018-09-14.09:42:49.758>
    created_at = <Date 2018-03-13.21:25:05.136>
    labels = ['3.8', 'type-feature', 'library']
    title = 'Add as_integer_ratio() to int() objects'
    updated_at = <Date 2018-10-19.21:46:35.061>
    user = 'https://github.com/rhettinger'

    bugs.python.org fields:

    activity = <Date 2018-10-19.21:46:35.061>
    actor = 'vstinner'
    assignee = 'lisroach'
    closed = True
    closed_date = <Date 2018-09-14.09:42:49.758>
    closer = 'rhettinger'
    components = ['Library (Lib)']
    creation = <Date 2018-03-13.21:25:05.136>
    creator = 'rhettinger'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33073
    keywords = ['patch']
    message_count = 26.0
    messages = ['313780', '313781', '313782', '313783', '313784', '313785', '313788', '313791', '313794', '313795', '313877', '313891', '313892', '313896', '313898', '313937', '313939', '321538', '321637', '321719', '321729', '321739', '325328', '325336', '325338', '328064']
    nosy_count = 9.0
    nosy_names = ['tim.peters', 'rhettinger', 'mark.dickinson', 'vstinner', 'njs', 'serhiy.storchaka', 'Eric.Wieser', 'Nofar Schnider', 'lisroach']
    pr_nums = ['8750', '9297', '9300', '9303']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue33073'
    versions = ['Python 3.8']

    @rhettinger
    Copy link
    Contributor Author

    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?

    @rhettinger rhettinger added 3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Mar 13, 2018
    @tim-one
    Copy link
    Member

    tim-one commented Mar 13, 2018

    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.

    @mdickinson
    Copy link
    Member

    On the Fraction constructor, it looks like there's some overlap with the goals of issue bpo-28716 here.

    @rhettinger
    Copy link
    Contributor Author

    it looks like there's some overlap with the goals of issue bpo-28716

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

    @Shredder13
    Copy link
    Mannequin

    Shredder13 mannequin commented Mar 13, 2018

    I'm working on it

    @serhiy-storchaka
    Copy link
    Member

    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.

    @tim-one
    Copy link
    Member

    tim-one commented Mar 13, 2018

    Serhiy, we already went down the path of implementing this as a method. Of course numbers.Rational could define the method as return (self.numerator, self.denominator) for itself and its subclasses. Unless I'm confused, that would "magically" define the method for (at least) ints and Fractions too - but I didn't try it.

    @gvanrossum
    Copy link
    Member

    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.

    @tim-one
    Copy link
    Member

    tim-one commented Mar 13, 2018

    Thanks, Guido! I figured I was missing something :-)

    It looks like numbers.Rational is a "for real" base class of fractions.Fraction, though, so I'm in favor of supplying a default implementation of .as_integer_ratio() in numbers.Rational anyway. That would make a clear statement of intent, and would do the right thing for Fraction instances.

    @gvanrossum
    Copy link
    Member

    +1.

    On Tue, Mar 13, 2018, 16:26 Tim Peters <report@bugs.python.org> wrote:

    Tim Peters tim@python.org added the comment:

    Thanks, Guido! I figured I was missing something :-)

    It looks like numbers.Rational is a "for real" base class of
    fractions.Fraction, though, so I'm in favor of supplying a default
    implementation of .as_integer_ratio() in numbers.Rational anyway. That
    would make a clear statement of intent, and would do the right thing for
    Fraction instances.

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue33073\>


    @serhiy-storchaka
    Copy link
    Member

    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.

    @tim-one
    Copy link
    Member

    tim-one commented Mar 15, 2018

    Serhiy, I don't understand. If numbers.Rational is in fact a superclass of numpy.int64, then the latter will inherit an implementation added to the former. The idea here isn't to add an abstract method to the Rational interface, but a concrete default implementation:

    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?

    @mdickinson
    Copy link
    Member

    It's a "virtual" subclass. The numpy.integer parent class is registered here:

    https://github.com/numpy/numpy/blob/10ccfe747a68d974ff16a5c0765054b816d5486f/numpy/core/numerictypes.py#L944

    @tim-one
    Copy link
    Member

    tim-one commented Mar 15, 2018

    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?

    @mdickinson
    Copy link
    Member

    [Tim]

    [...] what would _you_ like to see done here?

    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.

    @njsmith
    Copy link
    Contributor

    njsmith commented Mar 16, 2018

    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)

    @njsmith
    Copy link
    Contributor

    njsmith commented Mar 16, 2018

    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.

    @vstinner
    Copy link
    Member

    I remove the "easy (C)" keyword since the feature seems to be controversal (at least, not so easy).

    @gvanrossum
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

    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.

    @vstinner vstinner removed the easy label Jul 16, 2018
    @rhettinger
    Copy link
    Contributor Author

    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.

    @rhettinger
    Copy link
    Contributor Author

    Nofar, do you want to continue to work on this or should I reassign?

    @rhettinger rhettinger assigned lisroach and unassigned Shredder13 Jul 24, 2018
    @rhettinger
    Copy link
    Contributor Author

    New changeset 5ac7043 by Raymond Hettinger (Lisa Roach) in branch 'master':
    bpo-33073: Adding as_integer_ratio to ints. (GH-8750)
    5ac7043

    @serhiy-storchaka
    Copy link
    Member

    The merged PR contains several errors. See comments on GitHub.

    @rhettinger
    Copy link
    Contributor Author

    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.

    @vstinner
    Copy link
    Member

    New changeset b2e2025 by Victor Stinner (Serhiy Storchaka) in branch 'master':
    bpo-33073: Rework int.as_integer_ratio() implementation (GH-9303)
    b2e2025

    @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
    3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants