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

Fraction wrongfully gets casted into float when given as argument to __rpow__ #119189

Closed
retooth2 opened this issue May 19, 2024 · 14 comments
Closed
Labels
type-bug An unexpected behavior, bug, or error

Comments

@retooth2
Copy link

retooth2 commented May 19, 2024

Bug report

Bug description:

When using the ** operator with a Fraction as a base and an object that implements __rpow__ as an exponent the fraction gets wrongfully casted into a float before being passed to __rpow__

from fractions import Fraction

foo = Fraction(4, 3)

class One:
    def __rpow__(self, other):
        return other

bar = foo**One()
print(bar)
print(type(bar))

Expected Output

4/3
<class 'fractions.Fraction'>

Actual Output:

1.3333333333333333
<class 'float'>

Tested with Python 3.12.3

CPython versions tested on:

3.12

Operating systems tested on:

macOS

Linked PRs

@retooth2 retooth2 added the type-bug An unexpected behavior, bug, or error label May 19, 2024
@zitterbewegung
Copy link
Contributor

zitterbewegung commented May 20, 2024

I'm investigating this.

@zitterbewegung
Copy link
Contributor

zitterbewegung commented May 20, 2024

Reproduced under Linux (Ubuntu). I am working on a Fix (I'm in the sprint workshop)

@MojoVampire
Copy link
Contributor

MojoVampire commented May 20, 2024

It's a mistake in the implementation of Fraction.__pow__; rather than returning NotImplemented when the type of the exponent is unrecognized and letting the other type handle it through __rpow__ normally, it intentionally does return float(a) ** b (where a is the name given to self).

It does something similar when a Fraction is raised to the power of another non-one denominator Rational (return float(a) ** float(b)), but it at least makes more sense there (the comments note that raising a fraction to a fraction generally produces an irrational number, which can't be represented as a fraction in the first place).

@MojoVampire
Copy link
Contributor

Is anyone working on a fix? If not, this is an easy one, I can do it.

I worry slightly about possible breakage of existing code, but:

  1. The existing behavior is a clear break from the operator overloading rules, and not one justified by anything special about fractions (unlike the fraction to fractional power case)
  2. The existing behavior loses data, while the fix would not, so if the existing behavior is desired, an __rpow__ provider can always revert to the old behavior manually (they don't even need it to be version-specific; if they detect a Fraction, which can't be given right now, they convert to float; on older Python, they'll never see a Fraction in the first place)
  3. I can't imagine many cases where this code path is exercised; if you're writing a type designed to work with numbers like this, it's almost always going to be part of the numeric tower, and should be able to handle other layers of the numeric tower appropriately.

@zitterbewegung
Copy link
Contributor

@MojoVampire I am working on a fix I'm in the cpython sprint.

@MojoVampire
Copy link
Contributor

MojoVampire commented May 20, 2024

@zitterbewegung: Same. I'm the guy standing up cause my back is borked. Fix is pretty easy:

        elif isinstance(b, (float, complex)):
            return float(a) ** b
        else:
            return NotImplemented

replacing the existing final line. Tests are always the pain. I'll review when you're done.

@retooth2
Copy link
Author

  1. I can't imagine many cases where this code path is exercised; if you're writing a type designed to work with numbers like this, it's almost always going to be part of the numeric tower, and should be able to handle other layers of the numeric tower appropriately.

For context: I stumbled over the bug when implementing a wrapper number class for exact precision arithmetic that holds the data as a symbolic expression (so e.g. irrational numbers can be used in calculations without error). The class is designed to work with all builtin number types, e.g.

a = Fraction(1, 3)
b = ExactNumber(Fraction(1, 2))
print(a**b)
ExactNumber(1/3**1/2)

My class is built on the sympy package that does similar things on its own, so this should also be relevant to them.
Anyway: Thanks for the quick response, I really appreciate it.

Best,
Fabian

@serhiy-storchaka
Copy link
Member

I worked on this today. The issue itself may be not difficult, but I noticed that tests do not cover many cases in mixed Fraction arithmetic. #119236 adds many new tests, so we will see what other side effect our changes can cause.

@zitterbewegung
Copy link
Contributor

@serhiy-storchaka I will look at this with @MojoVampire to see if there are any additional issues.

@MojoVampire
Copy link
Contributor

I'd suggest the news entry simplify to something like:

fractions.Fraction.__pow__ no longer coerces itself to float for unrecognized exponent types, instead returning NotImplemented to defer to the exponent's __rpow__ as normal.

Keeps it focused on what was fixed.

@zitterbewegung
Copy link
Contributor

zitterbewegung commented May 20, 2024

With this fix I see a test failure when manually putting in the new test_fractions code in
#119236

======================================================================
FAIL: testMixedPower (test.test_fractions.FractionTest.testMixedPower)

Traceback (most recent call last):
File "/Users/r2q2/Projects/cpython/Lib/test/test_fractions.py", line 903, in testMixedPower
self.assertTypedEquals(F(3, 2) ** Rect(2, 0), Polar(2.25, 0.0))
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/r2q2/Projects/cpython/Lib/test/test_fractions.py", line 282, in assertTypedEquals
self.assertEqual(expected, actual)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
AssertionError: Polar(Fraction(9, 4), 0.0) != Polar(2.25, 0.0)


Ran 45 tests in 0.038s

FAILED (failures=1)
test test_fractions failed
test_fractions failed (1 failure)

== Tests result: FAILURE ==

1 test failed:
test_fractions

Total duration: 97 ms
Total tests: run=45 failures=1
Total test files: run=1/1 failed=1
Result: FAILURE

@MojoVampire
Copy link
Contributor

Looks like a mistake in the test (or if you prefer, a test tailored to the old behavior). The Rect defines:

    def __rpow__(self, other):
        return Polar(other ** self.x, math.log(other) * self.y)

and the test expects other ** self.x to produce a float, but given other is a Fraction, and self.x is an int (which Fraction can use losslessly to produce a new Fraction), the observed result is correct, post-patch.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 20, 2024
…GH-119236)

(cherry picked from commit fe67af1)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 20, 2024
…GH-119236)

(cherry picked from commit fe67af1)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue May 20, 2024
…9236) (GH-119256)

(cherry picked from commit fe67af1)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue May 20, 2024
…9236) (GH-119255)

(cherry picked from commit fe67af1)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue May 21, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 21, 2024
…thonGH-119298)

(cherry picked from commit 10b1bd9)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 21, 2024
…thonGH-119298)

(cherry picked from commit 10b1bd9)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka added a commit that referenced this issue May 21, 2024
…H-119298) (GH-119347)

(cherry picked from commit 10b1bd9)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@serhiy-storchaka
Copy link
Member

The new tests were purposed to fail with this change. They were added so that we can see what effect the change has and decide whether it is correct. If some visible changes are not correct, we should find other fixes for such cases.

I have other changes for mixed Fraction arithmetic, but I am not sure that they should be backported, so I'll create a separate PR.

serhiy-storchaka added a commit that referenced this issue May 22, 2024
…H-119298) (GH-119346)

(cherry picked from commit 10b1bd9)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka pushed a commit that referenced this issue May 31, 2024
When using the ** operator or pow() with Fraction as the base
and an exponent that is not rational, a float, or a complex, the
fraction is no longer converted to a float.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 31, 2024
When using the ** operator or pow() with Fraction as the base
and an exponent that is not rational, a float, or a complex, the
fraction is no longer converted to a float.
(cherry picked from commit b9965ef)

Co-authored-by: Joshua Herman <30265+zitterbewegung@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 31, 2024
When using the ** operator or pow() with Fraction as the base
and an exponent that is not rational, a float, or a complex, the
fraction is no longer converted to a float.
(cherry picked from commit b9965ef)

Co-authored-by: Joshua Herman <30265+zitterbewegung@users.noreply.github.com>
@serhiy-storchaka
Copy link
Member

See also #119838.

noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
When using the ** operator or pow() with Fraction as the base
and an exponent that is not rational, a float, or a complex, the
fraction is no longer converted to a float.
serhiy-storchaka pushed a commit that referenced this issue Jul 16, 2024
…-119835)

When using the ** operator or pow() with Fraction as the base
and an exponent that is not rational, a float, or a complex, the
fraction is no longer converted to a float.
(cherry picked from commit b9965ef)

Co-authored-by: Joshua Herman <30265+zitterbewegung@users.noreply.github.com>
serhiy-storchaka pushed a commit that referenced this issue Jul 16, 2024
…-119836)

When using the ** operator or pow() with Fraction as the base
and an exponent that is not rational, a float, or a complex, the
fraction is no longer converted to a float.
(cherry picked from commit b9965ef)

Co-authored-by: Joshua Herman <30265+zitterbewegung@users.noreply.github.com>
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
When using the ** operator or pow() with Fraction as the base
and an exponent that is not rational, a float, or a complex, the
fraction is no longer converted to a float.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants