feat(core): make sympy.Integer compatible with Integral ABC #21699
Conversation
|
✅ Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.9. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
|
As discussed in #19311 we shouldn't register with the ABC unless the methods of the ABC are actually supported: |
I guess I don't understand the rational of making that a requirement in light of the fact that existing registrations with the ABC were already kind of lies. Wouldn't this at least get closer to the intended truth? Or is it that the current registration is now technically correct (in light of #19311 (comment)), and all that remains for registering Is supporting bitwise functions even feasible in light of #19311 (comment) ? Forgive me if I don't appreciate all the implications how existing bitwise operators (e.g., |
Don't use one bug to justify introducing another! I don't understand why anyone cares about using these ABCs if they don't also care that registration with an ABC actually means something. The rest of the discussion should be at #19311 rather than here. |
I've commented on the issue |
Perhaps that was the source of my misunderstanding. I didn't think I was introducing another bug. I thought I was merely not completely fixing an existing one.
Fair. I have attempted to update the PR accordingly and in light of #19311 (comment). |
posita
changed the title
sympy/core/numbers.py
Outdated
| def __lshift__(self, other): | ||
| if global_parameters.evaluate: | ||
| if isinstance(other, int): | ||
| return Integer(self.p << other) | ||
| elif isinstance(other, Integer): | ||
| return Integer(self.p << other.p) | ||
| return NotImplemented |
There was a problem hiding this comment.
I don't think we need to check evaluate here since there is no unevaluated form. This would raise TypeError when evaluate=False which isn't particularly useful and probably contradicts the Integral ABC anyway.
I'd write this as:
if not isinstance(other, (int, Integer)):
return NotImplemented
return Integer(self.p << int(other))I'm not sure what the numbers ABC say about interaction between different implementations of each ABC. Should this method work for any numbers.Integral? If so then maybe it should be:
if not isinstance(other, (int, Integer, Integral)):
return NotImplemented
return Integer(self.p << int(other))There was a problem hiding this comment.
Oh! I like that much better. I took a stab with a slight twist on style. (I've developed a habitual nervousness around if not … because it tends to mess with my brain, even though it's perfectly readable in this case.) If you prefer the flow as you've written it, let me know and I'll change it. I also like including Integral, so I've done that as well.
Technically, one could probably get away with isinstance(other, (Integer, numbers.Integral)), since isinstance(int(0), numbers.Integral) should evaluate to True.
There was a problem hiding this comment.
Technically, one could probably get away with
isinstance(other, (Integer, numbers.Integral)), sinceisinstance(int(0), numbers.Integral)should evaluate toTrue.
Wouldn't the changes here also mean that isinstance(Integer(0), numbers.Integral would be True as well?
I doubt performance is an issue here (anyone looking for better performance should just use int rather than Integer) but testing isinstance with an ABC is slow so it's faster if you can shortcut the common cases:
In [119]: from numbers import Integral
In [120]: x = 1
In [121]: %timeit isinstance(x, int)
218 ns ± 0.917 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
In [122]: %timeit isinstance(x, Integral)
1.64 µs ± 74 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
In [123]: %timeit isinstance(x, (int, Integral))
269 ns ± 36.8 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)There was a problem hiding this comment.
Interesting. I could make one slight tweak to remove Integer from the isinstance checks in the __r…__ methods (to signal that other won't be an Integer on that path), but I think its presence is logically harmless.
This is an example of what I mean:
diff --git a/sympy/core/numbers.py b/sympy/core/numbers.py
index af20ef64e3..73f828de4e 100644
--- a/sympy/core/numbers.py
+++ b/sympy/core/numbers.py
@@ -2429,7 +2429,7 @@ def __lshift__(self, other):
return NotImplemented
def __rlshift__(self, other):
- if isinstance(other, (int, Integer, numbers.Integral)):
+ if isinstance(other, (int, numbers.Integral)):
return Integer(int(other) << self.p)
else:
return NotImplementedJust let me know what you'd prefer.
There was a problem hiding this comment.
My preference would be to test for Integer explicitly so that it is very clear that this works with Integer but honestly I don't think it matters much so I don't mind.
A more important confusion that someone would likely have when looking at this code is to wonder why sympify isn't being used. There should be a long comment above these methods saying something like:
Bitwise operations are defined for Integer only and not for general sympy expressions. This is for compatibility with the numbers.Integer ABC which only defines these operations between instances of numbers.Integral. These methods should check explicitly for integer types rather than using sympify because they should not accept arbitrary symbolic expressions and there is no symbolic analogue of the bitwise operations.
There was a problem hiding this comment.
Added. I tweaked the language slightly, but reproduced it largely verbatim.
There was a problem hiding this comment.
I don't think we need to check evaluate here since there is no unevaluated form.
>>> Rational(4, 8, gcd=1)
4/8
sympy/core/numbers.py
Outdated
| if isinstance(other, int): | ||
| return Integer(other << self.p) | ||
| elif isinstance(other, Integer): | ||
| return Integer(other.p << self.p) |
There was a problem hiding this comment.
Have you checked coverage of these methods? I think this is unreachable.
There was a problem hiding this comment.
UPDATE: Discussed in separate thread above.
Hmmm…now that you mention it, I think all we need is something like this:
def __lshift__(self, other):
if isinstance(other, numbers.Integral):
return Integer(self.p << int(other))
else:
return NotImplemented
def __rlshift__(self, other):
if isinstance(other, numbers.Integral):
return Integer(int(other) << self.p)
else:
return NotImplementedBoth int and Integer should be registered with numbers.Integral, so the above should be sufficient. Its symmetry is a tiny bit misleading, since, as you point out, the __r…__ methods will never be called where other is an Integer. If it's important, the following may be helpful in cultivating that understanding:
def __lshift__(self, other):
if isinstance(other, (numbers.Integral, Integer)): # <- Integer is technically unnecessary, but harmless here
return Integer(self.p << int(other))
else:
return NotImplemented
def __rlshift__(self, other):
if isinstance(other, numbers.Integral):
return Integer(int(other) << self.p)
else:
return NotImplemented
|
Are the other methods of An obvious difference would be: In [128]: Integer(1) / Integer(2)
Out[128]: 1/2
In [129]: int(1) / int(2)
Out[129]: 0.5All methods should be checked carefully |
Ah, yes. We do have this hint from PEP 238's "Semantics of True Division" section:
That being said, PEP 239 was rejected. Standard library implementations are slightly inconsistent with >>> from fractions import Fraction
>>> Fraction(1) / 4
Fraction(1, 4)
>>> Fraction(1) // 4
0
>>> Fraction(1) // Fraction(4)
0
>>> from decimal import Decimal
>>> Decimal("1.0") / 4
Decimal('0.25')
>>> Decimal("1.0") // 4
Decimal('0')
>>> Decimal("1.0") // Decimal("4.0")
Decimal('0')Given the ambiguities, I think returning a >>> simplify("1 / 4")
1/4
>>> simplify("1.0 / 4")
0.250000000000000
>>> simplify("1 // 4")
0
>>> simplify("1.0 // 4")
0With the exception of |
I think the appropriate PEP is PEP 3141 -- A Type Hierarchy for Numbers:
Note that Decimal is not included in the Real ABC: I guess that for true division we have The other methods listed for |
|
I wonder whether operations like |
|
Thanks for the continued attention. Hopefully this is close to acceptance.
PEP 3141 provides no guidance on division. IMO,
What does this mean? Do I need to write more tests? Am I waiting on you to check those methods? Are you waiting on me to provide something?
My preference is to leave such decisions to the caller rather than force them to deal with an implicit conversion. My recommendation is to live with it like this for awhile and change it in a subsequent PR if it seems important. |
In so far as there is a spec for what methods should exist and what they should return I would like you to check it. Where the spec is lacking at least the stdlib implementations of these classes can be compared with what sympy's Integer gives in this PR. I ask for this because I didn't personally see any value in supporting these ABCs at all (#12134 (comment)) but I ended up spending time fixing the bugs from the implementation to support the Rational ABC. I'm happy to merge this but please check it properly so that merging this can put the issue to rest without me needing to fill in the gaps in future. If this is something that you care about then please make sure it is done properly. This has no value to sympy internally but I understand that there are downstream use cases where this is somehow beneficial although apparently no one will answer what they are: #19311 (comment). |
|
I'm fine doing what is required to get this accepted, but first I need to understand what that is. I'm hoping this is responsive: In [26]: for binop in operator.add, operator.and_, operator.eq, operator.floordiv, operator.ge, operator.gt, operator.le, operator.lshift, operator.lt, operator.mod, operator.mul, operator.ne, operator.or_, operator.pow, operator.rshift, operator.sub, operator.truediv, operator.xor:
...: a, b = S("-12"), S("5")
...: r = binop(a, b)
...: print(f"{binop.__name__}({a!r}, {b!r}) -> {r!r} ({type(r)!r})")
add(-12, 5) -> -7 (<class 'sympy.core.numbers.Integer'>)
and_(-12, 5) -> 4 (<class 'sympy.core.numbers.Integer'>)
eq(-12, 5) -> False (<class 'bool'>)
floordiv(-12, 5) -> -3 (<class 'sympy.core.numbers.Integer'>)
ge(-12, 5) -> False (<class 'sympy.logic.boolalg.BooleanFalse'>)
gt(-12, 5) -> False (<class 'sympy.logic.boolalg.BooleanFalse'>)
le(-12, 5) -> True (<class 'sympy.logic.boolalg.BooleanTrue'>)
lshift(-12, 5) -> -384 (<class 'sympy.core.numbers.Integer'>)
lt(-12, 5) -> True (<class 'sympy.logic.boolalg.BooleanTrue'>)
mod(-12, 5) -> 3 (<class 'sympy.core.numbers.Integer'>)
mul(-12, 5) -> -60 (<class 'sympy.core.numbers.Integer'>)
ne(-12, 5) -> True (<class 'bool'>)
or_(-12, 5) -> -11 (<class 'sympy.core.numbers.Integer'>)
pow(-12, 5) -> -248832 (<class 'sympy.core.numbers.Integer'>)
rshift(-12, 5) -> -1 (<class 'sympy.core.numbers.NegativeOne'>)
sub(-12, 5) -> -17 (<class 'sympy.core.numbers.Integer'>)
truediv(-12, 5) -> -12/5 (<class 'sympy.core.numbers.Rational'>)
xor(-12, 5) -> -15 (<class 'sympy.core.numbers.Integer'>)
In [27]: for unop in complex, int, float, round, math.ceil, math.floor, math.trunc, operator.abs, operator.index, operator.invert, operator.neg, operator.pos:
...: a = S("-12")
...: r = unop(a)
...: print(f"{unop.__name__}({a!r}) -> {r!r} ({type(r)!r})")
complex(-12) -> (-12+0j) (<class 'complex'>)
int(-12) -> -12 (<class 'int'>)
float(-12) -> -12.0 (<class 'float'>)
round(-12) -> -12 (<class 'sympy.core.numbers.Integer'>)
ceil(-12) -> -12 (<class 'sympy.core.numbers.Integer'>)
floor(-12) -> -12 (<class 'sympy.core.numbers.Integer'>)
trunc(-12) -> -12 (<class 'sympy.core.numbers.Integer'>)
abs(-12) -> 12 (<class 'sympy.core.numbers.Integer'>)
index(-12) -> -12 (<class 'int'>)
invert(-12) -> 11 (<class 'sympy.core.numbers.Integer'>)
neg(-12) -> 12 (<class 'sympy.core.numbers.Integer'>)
pos(-12) -> -12 (<class 'sympy.core.numbers.Integer'>)
In [28]: for binop in "+", "&", "==", "//", ">=", ">", "<=", "<<", "<", "%", "*", "!=", "|", "**", ">>", "-", "/", "^":
...: s = "-12 {} 5".format(binop)
...: r = S(s)
...: print(f"{s} -> {r!r} ({type(r)!r})")
-12 + 5 -> -7 (<class 'sympy.core.numbers.Integer'>)
-12 & 5 -> 4 (<class 'sympy.core.numbers.Integer'>)
-12 == 5 -> False (<class 'bool'>)
-12 // 5 -> -3 (<class 'sympy.core.numbers.Integer'>)
-12 >= 5 -> False (<class 'sympy.logic.boolalg.BooleanFalse'>)
-12 > 5 -> False (<class 'sympy.logic.boolalg.BooleanFalse'>)
-12 <= 5 -> True (<class 'sympy.logic.boolalg.BooleanTrue'>)
-12 << 5 -> -384 (<class 'sympy.core.numbers.Integer'>)
-12 < 5 -> True (<class 'sympy.logic.boolalg.BooleanTrue'>)
-12 % 5 -> 3 (<class 'sympy.core.numbers.Integer'>)
-12 * 5 -> -60 (<class 'sympy.core.numbers.Integer'>)
-12 != 5 -> True (<class 'bool'>)
-12 | 5 -> -11 (<class 'sympy.core.numbers.Integer'>)
-12 ** 5 -> -248832 (<class 'sympy.core.numbers.Integer'>)
-12 >> 5 -> -1 (<class 'sympy.core.numbers.NegativeOne'>)
-12 - 5 -> -17 (<class 'sympy.core.numbers.Integer'>)
-12 / 5 -> -12/5 (<class 'sympy.core.numbers.Rational'>)
-12 ^ 5 -> -248832 (<class 'sympy.core.numbers.Integer'>)
In [29]: for unop in "~", "+", "-":
...: s = "{}12".format(unop)
...: r = S(s)
...: print(f"{s} -> {r!r} ({type(r)!r})")
~12 -> -13 (<class 'sympy.core.numbers.Integer'>)
+12 -> 12 (<class 'sympy.core.numbers.Integer'>)
-12 -> -12 (<class 'sympy.core.numbers.Integer'>)
In [30]: for unop in "~", "+", "-":
...: s = "{}-12".format(unop)
...: r = S(s)
...: print(f"{s} -> {r!r} ({type(r)!r})")
~-12 -> 11 (<class 'sympy.core.numbers.Integer'>)
+-12 -> -12 (<class 'sympy.core.numbers.Integer'>)
--12 -> 12 (<class 'sympy.core.numbers.Integer'>)If the above does not meet the request, I likely (still) misunderstood it. If so, can you please let me know what's missing? |
I just wanted you to say "yes, I've checked all the methods" :) I think that this looks good. I'll give some time to see if there are other reviews but otherwise I think this is good to merge. |
|
I commented above to show that unevaluated Rationals are possible: >>> Rational(4,8,gcd=1)
4/8 |
It's not unevaluated Rationals but rather unevaluated bitwise operations. SymPy has no |
|
What are maintainers' preferences for squashing/rebasing of an in-flight PR? I recently rebased to |
|
I don't think rebasing is problematic for reviewers. The way I check out a branch for review is just (need to adjust git config to make this work) and this command will happily update to the latest in the PR if it gets force pushed. I think that this is small enough and simple enough for one commit although the 4 commits you have here are fine except for the commit messages:
As a general rule I think that "Respond to PR feedback" is not a good commit message because it's useless when looking at either the blame or the log: I would say squash this to one commit with a message like: |
Make `sympy.Integer` compatible with the `numbers.Integral` ABC as defined
in PEPs 3141 and 238. (See also PEP 239.) The necessary bitwise operations
(e.g. `1 << 2`) are added to `sympy.Integer` and `sympy.Integer` is
registered with the `Integral` ABC such that:
isinstance(sympify(1), Integral) -> True
Prior to this commit, `sympy.Integer.__truediv__` and
`sympy.Integer.__rtruediv__` already returned `sympy.Rational` values,
which is not only ergonomic, but also appears compliant.
Fix #19311.
posita
changed the title
|
Commit history cleaned up. Please let me know if there's anything out of place, and thanks for the guidance! |
|
We register the classes because it is faster but you should be able to temporarily switch it to subclassing and that will tell you if any methods are missing. |
Doesn't work because of metaclass issues: |
|
Looks good, merging. Thanks! |
Make
sympy.Integercompatible with thenumbers.IntegralABC as definedin PEPs 3141 and 238. (See also PEP 239.) The necessary bitwise operations
(e.g.
1 << 2) are added tosympy.Integerandsympy.Integerisregistered with the
IntegralABC such that:Prior to this commit,
sympy.Integer.__truediv__andsympy.Integer.__rtruediv__already returnedsympy.Rationalvalues,which is not only ergonomic, but also appears compliant.
Fix #19311.