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

negative Fraction ** negative int not normalized #71726

Closed
vedgar mannequin opened this issue Jul 17, 2016 · 16 comments
Closed

negative Fraction ** negative int not normalized #71726

vedgar mannequin opened this issue Jul 17, 2016 · 16 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@vedgar
Copy link
Mannequin

vedgar mannequin commented Jul 17, 2016

BPO 27539
Nosy @rhettinger, @terryjreedy, @mdickinson, @serhiy-storchaka, @vedgar
Files
  • TwCVpiFp.diff
  • issue_27539_fix.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/mdickinson'
    closed_at = <Date 2016-08-22.09:57:55.708>
    created_at = <Date 2016-07-17.07:32:11.692>
    labels = ['type-bug', 'library']
    title = 'negative Fraction ** negative int not normalized'
    updated_at = <Date 2016-08-22.09:57:55.706>
    user = 'https://github.com/vedgar'

    bugs.python.org fields:

    activity = <Date 2016-08-22.09:57:55.706>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2016-08-22.09:57:55.708>
    closer = 'mark.dickinson'
    components = ['Library (Lib)']
    creation = <Date 2016-07-17.07:32:11.692>
    creator = 'veky'
    dependencies = []
    files = ['43786', '44179']
    hgrepos = []
    issue_num = 27539
    keywords = ['patch']
    message_count = 16.0
    messages = ['270616', '270750', '270808', '270812', '271023', '271060', '271067', '272471', '272528', '273282', '273284', '273285', '273286', '273296', '273345', '273346']
    nosy_count = 6.0
    nosy_names = ['rhettinger', 'terry.reedy', 'mark.dickinson', 'python-dev', 'serhiy.storchaka', 'veky']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27539'
    versions = ['Python 3.5', 'Python 3.6']

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Jul 17, 2016

    I already wrote http://bugs.python.org/msg270548, but can't seem to reopen the issue, so I think the best thing is to report the bug separately.

    So, in issue http://bugs.python.org/issue21136, performance enhancement https://hg.python.org/cpython/rev/91d7fadac271/ enabled a shortcut for some operations (pow among them) to avoid reducing the result to lowest terms if it can be concluded it's already reduced.

    However, the logic has a corner case that was handled incorrectly. If a negative Fraction is raised to a negative integer, the result is a Fraction with a negative denominator, which is not normalized and in fact breaks many other operations (which rightly assume their operands to be normalized).

    >>> import fractions
    >>> fractions.Fraction(-1, 2) ** -1
    Fraction(2, -1)
    >>> _ == -2
    False

    Of course, the easy fix would be to change line 52 of fractions.py to _normalize=True - but that would reintroduce the slowness talked about in http://bugs.python.org/issue21136#msg215409. Alternative fix is to branch depending on the sign of the base, which might be messy if we intend to be completely general -- for example, in finite quotient rings, fractions don't have well-defined signs.

    [BTW I'm not quite sure why the code in fractions.py is so general, with many weird cases trying to support every imaginable construction - maybe someone really wanted such a level of generality. I don't, so I'd be fine with this working only on ordinary int/int Fractions.]

    @vedgar vedgar mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jul 17, 2016
    @mdickinson
    Copy link
    Member

    Oops, that's an embarrassing bug.

    Branching depending on the sign sounds fine to me. There's no point worrying about supporting strange Fraction-like things without a concrete idea of exactly which such things we want to support.

    Are you interested in writing a patch?

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Jul 19, 2016

    Unfortunately, until now I was just a happy user of Python. I suppose some day I would have to give something back, and now is as good a day as any:-), but I have studied https://docs.python.org/devguide/ for a while and it seems really scary.

    It means I have to install and get acquainted with Mercurial [BTW I thought Python was on git since this year, it seems PEP-481 was just wishful thinking:(], and probably with Visual Studio since I use Windows - and I have never successfully installed that (I have some weird academic Windows licence it doesn't like, and it seems really hard to install for a single non-admin user, which is a must since this is actually not my machine:).

    So, let's put the cards on the table: if the idea of your post was to initiate me in the process of becoming a good open-source citizen, then I'll bite the bullet and try to do everything in my power to actually get my equipment into a state where I can contribute to Python - but it will take time. On the other hand, if the idea was to just fix the bug in time for 3.6b1, it would be a great deal simpler if you wrote the patch. :-/

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Jul 19, 2016

    If it is of any help, here is the patch I wrote online:

    http://www.mergely.com/TwCVpiFp/

    (also in attachment). I also took the liberty of properly isolating the _normalize argument by requiring it to be keyword-only (starting its name with underscore doesn't mean much if people could just call e.g. Fraction(2, 3, 4) without error).

    @terryjreedy
    Copy link
    Member

    Vedran: The migration to git and GitHub is in progress. The devguide is being moved today. CPython itself should be done by the end of the year, but after 3.6.0 is released in Sept.

    I am on Windows and yes, setting up on Windows can be a nuisance. Learning VS is not needed, as one can just run PCBuild/build.bat to compile. But installing 9 GB of even the free version, to use the compiler, is. TortoiseHG makes minimal use of hg pretty easy. I had never used any source control system before. But this will be unnecessary soon.

    I let Mark decide if he can use the diff as is. Are the line numbers those of fractions.py, or do they need to be translated?

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Jul 23, 2016

    The .diff format of patch for fractions.py is at
    http://bugs.python.org/file43786/TwCVpiFp.diff

    The line numbers in it are the newest I could get (from
    https://hg.python.org/cpython/file/default/Lib/fractions.py).

    I would also add the test I was talking about (Fraction(-1, 2) ** -1), if I knew where to do it.

    @terryjreedy
    Copy link
    Member

    Tests for fractions are in Lib/test/test_fractions.py. In necessary, post the new code in a message.

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Aug 11, 2016

    Here is the code for the patch:

    84c84
    < def __new__(cls, numerator=0, denominator=None, _normalize=True):
    ---

    def \_\_new__(cls, numerator=0, denominator=None, \*, \_normalize=True):
    

    459,466c459,466
    < if power >= 0:
    < return Fraction(a._numerator ** power,
    < a._denominator ** power,
    < _normalize=False)
    < else:
    < return Fraction(a._denominator ** -power,
    < a._numerator ** -power,
    < _normalize=False)
    ---

                num = a.\_numerator
                den = a.\_denominator
                if power \< 0:
                    num, den, power = den, num, -power
                    if den \< 0:
                        num = -num
                        den = -den
                return Fraction(num ** power, den ** power, \_normalize=False)
    

    I tried to add the test to test_fractions.py, but unfortunately unittest reports 3 unrelated failures due to mismatch in error messages (why does it check exact messages of exceptions anyway?). But it should just be adding

        self.assertTypedEquals(F(-1, 2), F(-2) ** -1)
    

    after line 408.

    _Please_, can this go in before 15th?

    @mdickinson
    Copy link
    Member

    _Please_, can this go in before 15th?

    Note that this is a bugfix, not a new feature, so it can still go in after the 15th.

    @mdickinson mdickinson self-assigned this Aug 21, 2016
    @mdickinson
    Copy link
    Member

    Here's a patch (against the 3.5 tip). I've dropped the keyword-only _normalize change; I have no objections to that change, but it's independent of the fix, and should probably be a 3.6-only change.

    @vedgar
    Copy link
    Mannequin Author

    vedgar mannequin commented Aug 21, 2016

    it's independent of the fix, and should probably be a 3.6-only change.

    Fair enough. Two questions:

    How/when does this appear in 3.6 line? I guess there is a standard timeline of how bugfixes are "forwardported", but I don't know what it is.

    How do I make the _normalize "kwonlyfication"? Do I report a new issue and refer to this one? I really think that accepting Fraction(2, 3, 4) is simply a bug (though I don't mind if it's fixed only in >=3.6).

    @mdickinson
    Copy link
    Member

    Do I report a new issue and refer to this one?

    Sure, that would work. A patch that included the change and a test that checks that Fraction(3, 4, True) raises TypeError would be good, too.

    @mdickinson
    Copy link
    Member

    How/when does this appear in 3.6 line?

    After allowing a couple of days for review, I'll commit the issue_27539_fix patch to the 3.5 branch, then immediately merge it to the 3.6 "default" branch. So that fix should appear in the upcoming 3.6 beta1.

    @serhiy-storchaka
    Copy link
    Member

    LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 22, 2016

    New changeset 7eea5b87f5fa by Mark Dickinson in branch '3.5':
    Issue bpo-27539: Fix unnormalised Fraction.__pow__ result for negative exponent and base. Thanks Vedran Čačić.
    https://hg.python.org/cpython/rev/7eea5b87f5fa

    New changeset 902e82f71880 by Mark Dickinson in branch 'default':
    Issue bpo-27539: Merge from 3.5.
    https://hg.python.org/cpython/rev/902e82f71880

    @mdickinson
    Copy link
    Member

    Thanks, all. Closing as fixed.

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants