SymPy numbers now inherit from Python numbers ABCs #16652

parkereberry1923 · 2019-04-15T00:44:52Z

References to other Issues or PRs

Fixes #12134

Brief description of what is fixed or changed

Number inherits from numbers.Number
Float inherits from numbers.Real
Rational inherits from numbers.Rational
Defined __floor__() and __ceil__() for Number, Float, and Rational
Defined numerator() and denominator() for Rational

Other comments

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

Release Notes

core
- isinstance for Python numeric ABCs now works for the basic SymPy number types.

sympy-bot · 2019-04-15T00:45:03Z

✅

Hi, I am the SymPy bot (v147). 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:

core
- isinstance for Python numeric ABCs now works for the basic SymPy number types. (#16652 by @EasyAsPi23, @parkereberry1923, and @shivanikohlii)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #12134


#### Brief description of what is fixed or changed
- Number inherits from numbers.Number
- Float inherits from numbers.Real
- Rational inherits from numbers.Rational
- Defined \_\_floor\_\_() and \_\_ceil\_\_() for Number, Float, and Rational
- Defined numerator() and denominator() for Rational

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

#### Release Notes
<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* core
  * `isinstance` for Python numeric ABCs now works for the basic SymPy number types.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

Sc0rpi0n101

This patch fails the build tests.
Not sure why #33122.12 failed

But you can try my suggestions and see if the tests pass

Sc0rpi0n101 · 2019-04-15T06:50:42Z

sympy/core/numbers.py


 import mpmath
 import mpmath.libmp as mlib
+import numbers


Suggested change

import numbers

import numbers as num

You might want to import it as num since you are working in a module of the same name

Sc0rpi0n101 · 2019-04-15T06:53:38Z

sympy/core/numbers.py

+    pass

-class Number(AtomicExpr):
+class Number(with_metaclass(ManagedABCMetaclass, AtomicExpr, numbers.Number)):


Try if this passes the tests

Suggested change

class Number(with_metaclass(ManagedABCMetaclass, AtomicExpr, numbers.Number)):

class Number(with_metaclass(ManagedABCMetaclass, AtomicExpr, num.Number)):

Maybe the build errors are because python thinks you are referencing sympy.code.number.Numbers inside the definition of the same, so the module cannot have Number as it is not defined yet.

Sc0rpi0n101 · 2019-04-15T06:54:28Z

sympy/core/numbers.py


-
-class Float(Number):
+class Float(with_metaclass(ManagedABCMetaclass, Number, numbers.Real)):


Suggested change

class Float(with_metaclass(ManagedABCMetaclass, Number, numbers.Real)):

class Float(with_metaclass(ManagedABCMetaclass, Number, num.Real)):

Same here

Sc0rpi0n101 · 2019-04-15T06:54:40Z

sympy/core/numbers.py



-class Rational(Number):
+class Rational(with_metaclass(ManagedABCMetaclass, Number, numbers.Rational)):


Suggested change

class Rational(with_metaclass(ManagedABCMetaclass, Number, numbers.Rational)):

class Rational(with_metaclass(ManagedABCMetaclass, Number, num.Rational)):

And here

codecov · 2019-04-15T16:49:57Z

Codecov Report

Merging #16652 into master will increase coverage by 0.061%.
The diff coverage is 95%.

@@             Coverage Diff              @@
##           master    #16652       +/-   ##
============================================
+ Coverage   73.79%   73.852%   +0.061%     
============================================
  Files         619       619               
  Lines      159222    159591      +369     
  Branches    37366     37454       +88     
============================================
+ Hits       117491    117862      +371     
+ Misses      36297     36272       -25     
- Partials     5434      5457       +23

Sc0rpi0n101 · 2019-04-15T19:25:24Z

>>> import numbers as nums
>>> nums.Number
<class 'numbers.Number'>
>>> class abc(nums.Number):
...     print ("hello")
... 
hello

How is the build getting an attribute error?
Number is a part of the module

well, you can try from numbers import Number as nums to see if that if that works

shivanikohlii · 2019-04-15T19:40:12Z

well, you can try from numbers import Number as nums to see if that works
Is there a way we can locally test these changes?

Sc0rpi0n101 · 2019-04-15T19:46:18Z

Is there a way we can locally test these changes?

Well, I tested the module core and it passed the tests.
I am running a full test for sympy. I will inform you if it fails.

For testing in sympy, you can check:
https://docs.sympy.org/latest/modules/utilities/runtests.html
https://github.com/sympy/sympy/wiki/Running-tests

or you can use pytest

shivanikohlii · 2019-04-15T19:47:56Z

I am running a full test for sympy. I will inform you if it fails.
I ran the test suite locally and it compiled without errors but is failing during integration.

Sc0rpi0n101 · 2019-04-15T20:03:09Z

tests finished: 8166 passed, 226 skipped, 354 expected to fail, 2 expected to fail but passed,
in 2253.24 seconds
Yeah, passed for my system too

Actually the tests didn't even start for the fails, the build failed before that.
Don't know why Travis is failing the build

Sc0rpi0n101 · 2019-04-15T20:04:33Z

Was Number not in python 2.7?
All the fails are 2.7 or pypy.
All 3.5 are passing?

shivanikohlii · 2019-04-15T20:06:11Z

Don't know why Travis is failing the build

Maybe @sylee957 might know what's causing it to fail.

Numbers existed in 2.7

Sc0rpi0n101 · 2019-04-15T20:12:41Z

Yeah, the 2.7 docs have numbers.Number as a base class.
Odd

Sc0rpi0n101

Can you try changing these and see if that work.

>>> from sympy import *
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sympy/__init__.py", line 57, in <module>
    from .core import *
  File "sympy/core/__init__.py", line 8, in <module>
    from .expr import Expr, AtomicExpr, UnevaluatedExpr
  File "sympy/core/expr.py", line 3616, in <module>
    from .mul import Mul
  File "sympy/core/mul.py", line 1805, in <module>
    from .numbers import Rational
  File "sympy/core/numbers.py", line 492, in <module>
    class Number(with_metaclass(ManagedABCMetaclass, AtomicExpr, nums.Number)):
AttributeError: 'module' object has no attribute 'Number'

I'm getting this in 2.7 on my machine too, did not get it in 3.7
Might be that using nums.Number in class definition was not supported. Not sure.

You can accept the changes in the review itself if it saves some time,
or we can wait for sylee

Sc0rpi0n101 · 2019-04-15T20:16:22Z

sympy/core/numbers.py


 import mpmath
 import mpmath.libmp as mlib
+import numbers as nums


Suggested change

import numbers as nums

from sympy.external import import_module

nums = import_module('numbers')

asmeurer · 2019-04-15T20:51:30Z

I think it would be better to register the classes with the abcs, similar to how numpy does it, rather than directly subclassing.

I'm not sure about bitwise operations. It could cause confusion with the logic operations that are defined for those operators. We do currently raise a TypeError when numbers other than 0 or 1 are used with boolean functions, so it isn't strictly an issue. See also #5353.

sylee957 · 2019-04-16T04:56:21Z

I think the reason of python 2 failing, is that

sympy/sympy/core/numbers.py

Line 22 in 47bef27

import numbers as nums

looking up the module sympy.core.numbers instead of python builtin package.

You can inspect what's going on, if you use print(help(nums)) anywhere in the file.

Adding from __future__ import absolute_import may help,
or you may use

from sympy.external import import_module
nums = import_module('numbers')

or anything you can use to load correct module.

Though I have not much knowledge, about the idea from @asmeurer though.

Sc0rpi0n101 · 2019-04-16T18:40:12Z

from sympy.external import import_module
nums = import_module('numbers')

Thanks, @sylee957
It did work locally for both python 2 and 3.
The import was successful and the tests also passed.

Still, have to see if it works for Travis

Sc0rpi0n101 · 2019-04-17T20:24:45Z

This time one of the build tests failed due to HTTP error.
Unfortunate.
Can you restart the build?

asmeurer · 2019-04-17T21:48:19Z

If we do go with subclassing instead of adding them to the list externally, we should determine if there is any performance impact (see https://github.com/sympy/sympy_benchmarks).

shivanikohlii · 2019-04-17T22:32:26Z

we should determine if there is any performance impact (see https://github.com/sympy/sympy_benchmarks).

How can we do that?

asmeurer · 2019-04-17T22:46:09Z

Apparently we need instructions in the README for that repo. You should set up asv and compare the results against master and this branch.

shivanikohlii · 2019-04-19T23:05:54Z

You should set up asv and compare the results against master and this branch.

I'm a little confused about how to use asv to check performance. I'm cloning sympy_benchmark and using that?

asmeurer · 2019-04-21T23:36:08Z

Yes. You'll need to edit the json file to change the repo from "repo": "https://github.com/sympy/sympy.git", to the path to your local checkout. Then you can do asv compare master Issue-12134-Python-Numbers-ABC.

shivanikohlii · 2019-04-25T15:54:08Z

asv compare master Issue-12134-Python-Numbers-ABC
>Did not find results for commit 54b108d354fe22023dccfca9a7f44c38b39a9881

I can't find this commit in our commit history, I'm not sure why asv compare master Issue-12134-Python-Numbers-ABC is looking for it. Is there a specific place I should be looking?

asmeurer · 2019-04-25T20:19:47Z

You have to run asv run -s 1 master and asv run -s 1 Issue-12134-Python-Numbers-ABC first so it can benchmark it. The asv syntax is a bit confusing. The README on the benchmarks page should be updated for this case. I wish asv would just run the benchmarks automatically when they aren't found.

Also, instead of master I would recommend using asv run -s1 54b108d354fe22023dccfca9a7f44c38b39a9881, the master commit that this was based on (for you, they are the same because you haven't pulled yet, but once you git pull the master branch they won't be). That way the benchmarks won't be affected by any unrelated changes that have been merged into master since you made this branch.

shivanikohlii · 2019-04-25T22:56:14Z

Thanks @asmeurer, that worked!

I guess it might make more sense to add them to the list externally instead of subclassing. What do you guys think?

asmeurer · 2019-04-26T00:15:12Z

I think it makes sense.

shivanikohlii · 2019-05-01T05:37:22Z

@asmeurer we did the register thing like numpy. Does this look good to go?

asmeurer · 2019-05-01T17:46:01Z

Can you add tests that the numbers ABCs work (like assert isinstance(Float(1.1), Real) and so on).

sylee957 · 2019-05-04T10:41:24Z

I wonder how the new implementation improves the performance. Do you have any benchmark result?

shivanikohlii · 2019-05-05T00:30:53Z

Do you have any benchmark result?

Yes, no decline in performance -

shivanikohlii · 2019-05-05T21:50:55Z

@sylee957 is there anything else we need to do for this PR?

asmeurer · 2019-05-08T21:16:36Z

sympy/core/numbers.py

+    nums.Rational.register(Rational)
+    nums.Rational.register(Integer)
+
+_register_classes();


Remove the semicolon from the end of the line.

asmeurer · 2019-05-08T21:18:41Z

sympy/core/numbers.py

 from sympy.core.cache import lru_cache
-
+from sympy.external import import_module
+nums = import_module('numbers')


Is there a reason this isn't just import numbers?

Yep, it fails on 2.7 due to name collision.

Oh I see. I think it should work if you add absolute_import to the __future__ import at the top of the file.

asmeurer · 2019-05-08T21:21:28Z

A couple of minor comments. The rest looks good.

…arkereberry1923/sympy into Issue-12134-Python-Numbers-ABC

shivanikohlii · 2019-05-09T00:37:52Z

@asmeurer Done! Thank you for telling me about the import fix.

asmeurer · 2019-05-09T01:39:46Z

Thank you. This looks good now. I updated the release notes since we aren't literally subclassing anymore.

ParksGames added 2 commits April 14, 2019 18:14

SymPy numbers now inherit from Python numbers ABCs

7d3e6e7

Remove unnecessary comments

410a286

Sc0rpi0n101 requested changes Apr 15, 2019

View reviewed changes

sylee957 added the core label Apr 15, 2019

Changes made to variable name to prevent build failures

47bef27

Sc0rpi0n101 requested changes Apr 15, 2019

View reviewed changes

Importing differently to fix python 2 error

25a698f

sylee957 mentioned this pull request Apr 21, 2019

Improve readme sympy/sympy_benchmarks#52

Merged

shivanikohlii added 2 commits April 30, 2019 16:22

Externally import numbers

9ce9e84

Hopefully fix 2.7 error

6786aea

shivanikohlii added 2 commits May 3, 2019 11:28

tests for sympy_abc

da54119

code_quaality issue

7b541cb

asmeurer reviewed May 8, 2019

View reviewed changes

Update numbers.py

0f618bc

shivanikohlii added 2 commits May 8, 2019 16:40

changing how to import to sympy

58d05f1

Merge branch 'Issue-12134-Python-Numbers-ABC' of https://github.com/p…

1175c13

…arkereberry1923/sympy into Issue-12134-Python-Numbers-ABC

asmeurer merged commit 09786a1 into sympy:master May 9, 2019

shivanikohlii deleted the Issue-12134-Python-Numbers-ABC branch May 9, 2019 01:42

sylee957 mentioned this pull request May 14, 2020

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

Closed

	class Number(with_metaclass(ManagedABCMetaclass, AtomicExpr, numbers.Number)):
	class Number(with_metaclass(ManagedABCMetaclass, AtomicExpr, num.Number)):



		class Float(Number):
		class Float(with_metaclass(ManagedABCMetaclass, Number, numbers.Real)):



		class Rational(Number):
		class Rational(with_metaclass(ManagedABCMetaclass, Number, numbers.Rational)):

	import numbers as nums
	from sympy.external import import_module
	nums = import_module('numbers')

Uh oh!

SymPy numbers now inherit from Python numbers ABCs #16652

SymPy numbers now inherit from Python numbers ABCs #16652

Uh oh!

Conversation

parkereberry1923 commented Apr 15, 2019 • edited by asmeurer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

Uh oh!

sympy-bot commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sc0rpi0n101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Sc0rpi0n101 commented Apr 15, 2019

Uh oh!

shivanikohlii commented Apr 15, 2019

Uh oh!

Sc0rpi0n101 commented Apr 15, 2019

Uh oh!

shivanikohlii commented Apr 15, 2019

Uh oh!

Sc0rpi0n101 commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sc0rpi0n101 commented Apr 15, 2019

Uh oh!

shivanikohlii commented Apr 15, 2019

Uh oh!

Sc0rpi0n101 commented Apr 15, 2019

Uh oh!

Sc0rpi0n101 left a comment

Choose a reason for hiding this comment

Uh oh!

Sc0rpi0n101 Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmeurer commented Apr 15, 2019

Uh oh!

sylee957 commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sc0rpi0n101 commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sc0rpi0n101 commented Apr 17, 2019

Uh oh!

asmeurer commented Apr 17, 2019

Uh oh!

shivanikohlii commented Apr 17, 2019

Uh oh!

asmeurer commented Apr 17, 2019

Uh oh!

shivanikohlii commented Apr 19, 2019

Uh oh!

asmeurer commented Apr 21, 2019

Uh oh!

shivanikohlii commented Apr 25, 2019

Uh oh!

asmeurer commented Apr 25, 2019

Uh oh!

shivanikohlii commented Apr 25, 2019

Uh oh!

parkereberry1923 commented Apr 15, 2019 •

edited by asmeurer

Loading

sympy-bot commented Apr 15, 2019 •

edited

Loading

codecov bot commented Apr 15, 2019 •

edited

Loading

Sc0rpi0n101 commented Apr 15, 2019 •

edited

Loading

Sc0rpi0n101 Apr 15, 2019 •

edited

Loading

sylee957 commented Apr 16, 2019 •

edited

Loading

Sc0rpi0n101 commented Apr 16, 2019 •

edited

Loading

shivanikohlii commented May 5, 2019 •

edited

Loading