bpo-39274: Ensure `Fraction.bool` returns a bool #18017

seberg · 2020-01-15T22:47:22Z

Some numerator types used (specifically NumPy) decides to not
return a python boolean for the != operation. Using the equivalent
call to bool() guarantees a bool return also for such types.

Closes bpo-39274

https://bugs.python.org/issue39274

vstinner · 2020-01-15T23:35:33Z

Lib/test/test_fractions.py

+                return bool(self.value)
+
+        r = F(1, 1)
+        r._numerator = custom_value(1)


I would prefer to avoid accessing private attribute _numerator:

numerator = custom_value(1) r = F(numerator, 1) ... self.assertEqual(numerator.bool_used, True)

(same below)

I agree fixed, I just got lost a bit because I missed that numerator.numerator is used (which loses the original type), but since its only that it seems nicer. Of course if the instantiation logic changes, there is a chance of the test needing to be revised.

Lib/fractions.py

Misc/NEWS.d/next/Library/2020-01-15-23-13-03.bpo-39274.lpc0-n.rst

vstinner · 2020-01-16T07:54:24Z

Lib/test/test_fractions.py

+                return self
+
+        numerator = custom_value(1)
+        r = F(numerator)


I would prefer to explicit the denominator: F(numerator, 1). Same below.

That does not change much, if I add that, the only difference is that I also have to define __mul__. The only solution is going back to r._numerator = ... (it is used elsewhere in the file)

vstinner · 2020-01-16T07:57:08Z

Lib/test/test_fractions.py

+
+            def __bool__(self):
+                self.bool_used = True
+                return bool(self.value)


The bug is that "numerator != 0" returns an object which is not a subtype of bool. Does your test reproduce the bug if you revert the fractions.py change?

True, I am testing that r._numerator.__bool__ is called, which is not quite the same thing of course. Other solutions to achieve the same things (e.g. bool(a._numerator != 0) are possible and would fail the test as well).
I can add __ne__ that does the bad thing, but it would never get called. This forces to adapt the test in case the code is adapted, but without a full fledged numpy integer around, it seems hard to write a generic test that only checks for a bool result.

Well, tried to make it a bit more waterproof (by resetting bool_used), if I can import NumPy – skipping the test when its not available – that may be nicer of course (but I assume that is a no-go). Otherwise it feels like I would have to implement a full fleged rational with added strange logic...

Its over engineered, but right now I do not have a better idea TBH (unless we just skip the test...)

You cannot use 3rd party modules like numpy in the Python test suite, sadly. (At least, we are trying hard to avoid that.)

So you should try to mimick numpy's bool behavior. Please just implement a ne() method which raises an AssertionError. It would make me more comfortable. Or better: implement eq and lt, and @ @functools.total_ordering:
https://docs.python.org/dev/library/functools.html

Again, these methods would raise an exception.

Next try... Had tried to avoid registering with numbers.Rational but there is probably no reason, so now not inheriting from int anymore, which is much cleaner (and added your error, instead of ensuring __bool__ is called).

Overall, seems much better, since now it really is a crippled object that cannot do anything but what it is supposed to do...

vstinner · 2020-01-16T07:57:29Z

Lib/test/test_fractions.py

+            @property
+            def numerator(self):
+                # required to preserve `self` during instantiation
+                return self


I would prefer to remove that and use F(numerator, 1) instead.

vstinner · 2020-01-17T12:39:49Z

Lib/test/test_fractions.py

+
+            def __bool__(self):
+                self.bool_used = True
+                return bool(self.value)


You cannot use 3rd party modules like numpy in the Python test suite, sadly. (At least, we are trying hard to avoid that.)

So you should try to mimick numpy's bool behavior. Please just implement a ne() method which raises an AssertionError. It would make me more comfortable. Or better: implement eq and lt, and @ @functools.total_ordering:
https://docs.python.org/dev/library/functools.html

Again, these methods would raise an exception.

vstinner · 2020-01-17T12:40:42Z

Lib/test/test_fractions.py

+        r = F(numerator)
+        # ensure the numerator was not lost during instantiation:
+        self.assertTrue(r.numerator is numerator)
+        numerator.bool_used = False


I suggest self.assertFalse(numerator.bool_used) instead.

Suggested change

numerator.bool_used = False

self.assertFalse(numerator.bool_used)

vstinner · 2020-01-17T12:40:58Z

Lib/test/test_fractions.py

+        numerator.bool_used = False
+        self.assertTrue(bool(r) is True)
+        # If bool(numerator) was called, a bool result should be guaranteed:
+        self.assertEqual(numerator.bool_used, True)


Suggested change

self.assertEqual(numerator.bool_used, True)

self.assertTrue(numerator.bool_used)

vstinner · 2020-01-17T12:41:29Z

Lib/test/test_fractions.py

+        # ensure the numerator was not lost during instantiation:
+        self.assertTrue(r.numerator is numerator)
+        numerator.bool_used = False
+        self.assertTrue(bool(r) is True)


To get the value if the test fails:

Suggested change

self.assertTrue(bool(r) is True)

self.assertIs(bool(r), True)

vstinner · 2020-02-03T08:36:29Z

@seberg: Did you see my latest comments? Would you mind to update your PR?

Some numerator types used (specifically NumPy) decides to not return a python boolean for the `!=` operation. Using the equivalent call to `bool()` guarantees a bool return also for such types. Closes bpo-39274

seberg · 2020-02-03T17:57:33Z

Thanks for the ping, sorry, I had somehow forgotten/missed the very true comments regarding the assert statements. (updated now)

codecov · 2020-02-03T18:26:23Z

Codecov Report

Merging #18017 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18017      +/-   ##
==========================================
- Coverage   82.13%   82.11%   -0.02%     
==========================================
  Files        1953     1954       +1     
  Lines      582680   583290     +610     
  Branches    44349    44407      +58     
==========================================
+ Hits       478602   478993     +391     
- Misses      94416    94658     +242     
+ Partials     9662     9639      -23

Impacted Files	Coverage Δ
Lib/test/test_binhex.py	`13.33% <0.00%> (-79.78%)`	⬇️
Lib/binhex.py	`17.84% <0.00%> (-58.49%)`	⬇️
Lib/test/test_distutils.py	`66.66% <0.00%> (-4.77%)`	⬇️
Lib/concurrent/futures/thread.py	`64.70% <0.00%> (-3.52%)`	⬇️
Lib/concurrent/futures/process.py	`55.72% <0.00%> (-1.47%)`	⬇️
Objects/codeobject.c	`82.59% <0.00%> (-1.31%)`	⬇️
Lib/functools.py	`95.46% <0.00%> (-0.82%)`	⬇️
Modules/binascii.c	`86.77% <0.00%> (-0.55%)`	⬇️
Lib/json/__init__.py	`92.30% <0.00%> (-0.45%)`	⬇️
Modules/_pickle.c	`74.73% <0.00%> (-0.34%)`	⬇️
... and 124 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc0284e...a465dad. Read the comment docs.

vstinner · 2020-02-05T17:34:04Z

Lib/test/test_fractions.py

+            __lt__ = __eq__
+
+        # We did not implement all abstract methods, so register:
+        numbers.Rational.register(CustomValue)


This may fail if you run the test with reference leak test:

./python -m test -R 3:3 test_fractions

Can you check that it does not leak? I would prefer to avoid .register() if it does leak.

... This test is getting more and more complex... Maybe we should just ignore the test and just merge the fix.

What do you think @mdickinson?

Hmmm, seems to work:

% /home/sebastian/python-dev/bin/python3.9d -m test -R 3:3 test_fractions 0:00:00 load avg: 0.39 Run tests sequentially 0:00:00 load avg: 0.39 [1/1] test_fractions beginning 6 repetitions 123456 ...... == Tests result: SUCCESS == 1 test OK. Total duration: 221 ms Tests result: SUCCESS

I have not used the python refcount checker, so not sure if it clears ABCs are not.

The fix itself looks good to me; I think we should merge as is.

vstinner · 2020-02-06T14:55:12Z

The fix itself looks good to me; I think we should merge as is.

Ok, I was maybe too far in term of nitpicking, sorry :-) It's very difficult to implement a type implementing the numbers.Rational ABC :-( I had exactly the same problem on another fractions bug, and I decided to not add such test: https://bugs.python.org/issue39350#msg361481

miss-islington · 2020-02-06T14:55:24Z

Thanks @seberg for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

Some numerator types used (specifically NumPy) decides to not return a Python boolean for the "a != b" operation. Using the equivalent call to bool() guarantees a bool return also for such types. (cherry picked from commit 427c84f) Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

bedevere-bot · 2020-02-06T14:55:52Z

GH-18376 is a backport of this pull request to the 3.8 branch.

miss-islington · 2020-02-06T14:56:54Z

Thanks @seberg for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

Some numerator types used (specifically NumPy) decides to not return a Python boolean for the "a != b" operation. Using the equivalent call to bool() guarantees a bool return also for such types. (cherry picked from commit 427c84f) Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

bedevere-bot · 2020-02-06T14:57:07Z

GH-18377 is a backport of this pull request to the 3.7 branch.

Some numerator types used (specifically NumPy) decides to not return a Python boolean for the "a != b" operation. Using the equivalent call to bool() guarantees a bool return also for such types. (cherry picked from commit 427c84f) Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>

seberg · 2020-02-06T21:20:15Z

Ah, nitpicking has its upsides :). If anything, maybe the next time there is a fraction issue, this actually helps writing a reasonable test for it.

the-knights-who-say-ni added the CLA signed label Jan 15, 2020

bedevere-bot added the awaiting review label Jan 15, 2020

vstinner reviewed Jan 15, 2020

View reviewed changes

seberg force-pushed the fraction-bool-return-bool branch from 56bdc21 to 89fc30c Compare January 15, 2020 23:53

vstinner reviewed Jan 16, 2020

View reviewed changes

seberg force-pushed the fraction-bool-return-bool branch 2 times, most recently from 08aee01 to 2413700 Compare January 16, 2020 17:17

vstinner reviewed Jan 17, 2020

View reviewed changes

seberg force-pushed the fraction-bool-return-bool branch from 2413700 to 4bf428a Compare January 17, 2020 15:43

BUG: Ensure Fraction.__bool__ returns a bool

a465dad

Some numerator types used (specifically NumPy) decides to not return a python boolean for the `!=` operation. Using the equivalent call to `bool()` guarantees a bool return also for such types. Closes bpo-39274

seberg force-pushed the fraction-bool-return-bool branch from 4bf428a to a465dad Compare February 3, 2020 17:55

vstinner reviewed Feb 5, 2020

View reviewed changes

vstinner merged commit 427c84f into python:master Feb 6, 2020

bedevere-bot removed the awaiting review label Feb 6, 2020

vstinner added the needs backport to 3.8 label Feb 6, 2020

bedevere-bot removed the needs backport to 3.8 label Feb 6, 2020

vstinner added the needs backport to 3.7 label Feb 6, 2020

bedevere-bot removed the needs backport to 3.7 label Feb 6, 2020

seberg deleted the fraction-bool-return-bool branch February 6, 2020 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-39274: Ensure `Fraction.bool` returns a bool #18017

bpo-39274: Ensure `Fraction.bool` returns a bool #18017

seberg commented Jan 15, 2020 •

edited by bedevere-bot

Loading

vstinner Jan 15, 2020

seberg Jan 15, 2020

vstinner Jan 16, 2020

seberg Jan 16, 2020

vstinner Jan 16, 2020

seberg Jan 16, 2020

seberg Jan 16, 2020

vstinner Jan 17, 2020

seberg Jan 17, 2020

vstinner Jan 16, 2020

vstinner Jan 17, 2020

vstinner Jan 17, 2020

vstinner Jan 17, 2020

vstinner Jan 17, 2020

vstinner commented Feb 3, 2020

seberg commented Feb 3, 2020

codecov bot commented Feb 3, 2020 •

edited

Loading

vstinner Feb 5, 2020

seberg Feb 5, 2020

mdickinson Feb 5, 2020

vstinner commented Feb 6, 2020

miss-islington commented Feb 6, 2020

bedevere-bot commented Feb 6, 2020

miss-islington commented Feb 6, 2020

bedevere-bot commented Feb 6, 2020

seberg commented Feb 6, 2020

	numerator.bool_used = False
	self.assertFalse(numerator.bool_used)

	self.assertEqual(numerator.bool_used, True)
	self.assertTrue(numerator.bool_used)

	self.assertTrue(bool(r) is True)
	self.assertIs(bool(r), True)

bpo-39274: Ensure Fraction.__bool__ returns a bool #18017

bpo-39274: Ensure Fraction.__bool__ returns a bool #18017

Conversation

seberg commented Jan 15, 2020 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Feb 3, 2020

seberg commented Feb 3, 2020

codecov bot commented Feb 3, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Feb 6, 2020

miss-islington commented Feb 6, 2020

bedevere-bot commented Feb 6, 2020

miss-islington commented Feb 6, 2020

bedevere-bot commented Feb 6, 2020

seberg commented Feb 6, 2020

bpo-39274: Ensure `Fraction.bool` returns a bool #18017

bpo-39274: Ensure `Fraction.bool` returns a bool #18017

seberg commented Jan 15, 2020 •

edited by bedevere-bot

Loading

codecov bot commented Feb 3, 2020 •

edited

Loading