sympy / sympy

A computer algebra system written in pure Python
https://sympy.org/
Other
12.8k stars 4.39k forks source link

bugs in PythonRationalType #5456

Open pernici opened 13 years ago

pernici commented 13 years ago
Hi,
there are a few bugs in PythonRationalType

>>> from sympy import *
>>> from sympy.polys.domains.pythonrationaltype import PythonRationalType
>>> print Rational(3)
3
>>> print PythonRationalType(3)
3/1
>>> a = Rational(3,2)
>>> Rational(a)
3/2
>>> a1 = PythonRationalType(3,2)
>>> PythonRationalType(a1)
PythonRationalType(1, 1)

I made a quick fix for these bugs in https://github.com/pernici/sympy/commit/a9a7381a087739f0650846abe1c61103586d1d6d to make the tests in https://github.com/sympy/sympy/pull/295 pass, however there are other bugs I did not fix

>>> PythonRationalType(a1,7)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sympy/polys/domains/pythonrationaltype.py", line 38, in __init__
    self.p = p//g
TypeError: unsupported operand type(s) for //: 'PythonRationalType' and 'int'

>>> b = Rational(4,5)
>>> Rational(a,b)
15/8
>>> b1 = PythonRationalType(4,5)
>>> PythonRationalType(a1,b1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sympy/polys/domains/pythonrationaltype.py", line 43, in __init__
    q = p.q*q
AttributeError: 'int' object has no attribute 'q'

By the way, what is the reason of having both Rational and
PythonRationalType?

Mario

Original issue for #5456: http://code.google.com/p/sympy/issues/detail?id=2357 Original author: https://code.google.com/u/107755593449647463741/

mattpap commented 13 years ago
Several bugs mentioned here are actually design decisions, e.g.:

>>> print PythonRationalType(3)
3/1

PythonRationalType is meant for internal purpose of sympy.polys, not for user convenience or looking good (Rational is for this). To comment on other issues, I will have to look into the pull request to see how PythonRationalType is used.

Original comment: http://code.google.com/p/sympy/issues/detail?id=2357#c1 Original author: https://code.google.com/u/101069955704897915480/

asmeurer commented 13 years ago
Another problem with PythonRationalType:

In [16]: S(PythonRationalType(2, 3))
Out[16]: 0.666666666666667

It should return Rational(2, 3).

gmpy does the same thing:

In [22]: QQ.dtype
Out[22]: <built-in function mpq>

In [23]: S(QQ(2, 3))
Out[23]: 0.666666666666667

**Status:** Accepted  
**Labels:** Polynomial  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2357#c2 Original author: https://code.google.com/u/asmeurer@gmail.com/

pernici commented 13 years ago
Hi Mateusz,
>Several bugs mentioned here are actually design decisions, e.g.:
>>> print PythonRationalType(3)
3/1
PythonRationalType is meant for internal purpose of sympy.polys, not for user convenience or looking good (Rational is for this).

In https://github.com/sympy/sympy/pull/295 I reduced the changes in PythonRationalType with respect to master; 
I gave up about having 3 instead of 3/1;
the only change is in __init__(self,p,q) when q is None, so the impact to performance should be very small, since nothing changes in the most frequent calls
self.__class__(p, q)  with q not None.

I find the QQ type to be very useful.

This is a bit off-topic, but
it would be nice if there where in Sympy other types like QQ, which have a
fast implementation if available, otherwise a Python type.
They should have some common properties, like if F is the constructor
a = F(a) if a does; F(1) is the multiplication identity in the ring, etc.
so that one can write algorithms working with generic rings.

For instance for finite fields it could work as in Sage
sage: F = GF(7)
sage: a = F(4)
sage: a + 3
0
(or is there in Sympy and I missed it?)

and with matrices
sage: M = MatrixSpace(QQ,2,2)
sage: M(1)
[1 0]
[0 1]

Mario

Original comment: http://code.google.com/p/sympy/issues/detail?id=2357#c3 Original author: https://code.google.com/u/107755593449647463741/

asmeurer commented 13 years ago
We do have Galios fields, but they are only internal in the polys.  See for example sympy/polys/domains/finitefield.py.  

For the matrices, hopefully that sort of thing will happen this summer with Sherjil's GSoC project.

Original comment: http://code.google.com/p/sympy/issues/detail?id=2357#c4 Original author: https://code.google.com/u/asmeurer@gmail.com/

asmeurer commented 12 years ago
**Status:** Valid  

Original comment: http://code.google.com/p/sympy/issues/detail?id=2357#c5 Original author: https://code.google.com/u/asmeurer@gmail.com/