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

"sympy.core.numbers.Integer" should be registered with "numbers.Integral" #19311

Closed
leycec opened this issue May 14, 2020 · 8 comments · Fixed by #21699
Closed

"sympy.core.numbers.Integer" should be registered with "numbers.Integral" #19311

leycec opened this issue May 14, 2020 · 8 comments · Fixed by #21699
Labels

Comments

@leycec
Copy link

leycec commented May 14, 2020

While recently perusing the sympy.core.numbers submodule, I espied with my rheumy little eye a trivial typo with respect to type registration. SymPy's Integer type is currently registered as satisfying the numbers.Rational API (which it obviously doesn't) (which is technically true but less useful), but should instead be registered as satisfying the numbers.Integral API (which it obviously does) (which is also true and much more useful): e.g.,

    # This less useful line...
    numbers.Rational.register(Integer)

    # ...should instead resemble this more useful line.
    numbers.Integral.register(Integer)

Thanks for all the symbolic maths, beloved SymPy volunteers! 💛

@FrozenBob
Copy link

SymPy's Integer type is currently registered as satisfying the numbers.Rational API (which it obviously doesn't)

numbers.Rational is a superclass of numbers.Integral, and anything that satisfies numbers.Integral satisfies numbers.Rational. For example, issubclass(int, numbers.Rational) is True.

The use of numbers.Rational.register(Integer) instead of numbers.Integral.register(Integer) is still probably a bug.

@leycec
Copy link
Author

leycec commented May 14, 2020

numbers.Rational is a superclass of numbers.Integral, and anything that satisfies numbers.Integral satisfies numbers.Rational.

Ah, yes. I'd forgotten that Python's "tower of numbers" is effectively inverted from intuition. For shame, me! Thanks for the friendly reminder that it is late and I should go to sleep, which I am now doing. 😴

The use of numbers.Rational.register(Integer) instead of numbers.Integral.register(Integer) is still probably a bug.

Right. As it currently stands, SymPy integers are rational but not integers from the non-SymPy perspective, which... absolutely must be wrong.

@sylee957
Copy link
Member

The issue came from implementing #16652

If we want Integer to inherit from numbers.Integral, we need bit-string operators: '<<', '>>', '|', '&', '^', and '~'.

And for reference to the comment, the bitwise operations should be defined, which may not be viable for sympy because sympy defines overrides bitwise operations for logical programming purposes.

@FrozenBob
Copy link

On one hand, that's definitely a compatibility issue. On the other hand, sympy.Integer doesn't support numerator or denominator either, but it's still registered with numbers.Rational. (sympy.Rational doesn't support numerator or denominator either.)

@sylee957 sylee957 added the core label May 14, 2020
@oscarbenjamin
Copy link
Contributor

Does anyone have any actual use for the Numbers ABCs?

@oscarbenjamin
Copy link
Contributor

sympy.Integer doesn't support numerator or denominator

That is fixed now.

@oscarbenjamin
Copy link
Contributor

Potentially we could just add the bitwise operations to sympy.Integer. Currently among sympy types the bitwise operations are defined for Boolean but not Expr. The potential point of confusion is the fact that Symbol is both Boolean and Expr (which I personally consider a contradiction) so e.g.:

In [114]: x, y = symbols('x, y')

In [115]: 1 << 2
Out[115]: 4

In [116]: (x << y).subs(x, 1).subs(y, 2)
Out[116]: True

Really though I think it's a mistake to use Symbol this way at all: there should be a BooleanSymbol class for use in Boolean contexts. That would take quite a while to fix but if we want to do that eventually then we can still add the bitwise operators to Integer now.

@oscarbenjamin
Copy link
Contributor

This is fixed now by #21699 which adds the bitwise methods to Integer and registers it with the numbers.Integral ABC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants