negative Fraction ** negative int not normalized

[vedgar]

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]

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.]

[mdickinson]

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]

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]

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]

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]

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]

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

[vedgar]

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]

_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]

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]

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]

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.