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

Making the module holonomic usable #11330

Merged
merged 16 commits into from
Jul 8, 2016
Merged

Conversation

shubhamtibra
Copy link
Contributor

@shubhamtibra shubhamtibra commented Jun 30, 2016

@shubhamtibra shubhamtibra changed the title Making the module holonomic usable [WIP] Making the module holonomic usable Jun 30, 2016
@shubhamtibra shubhamtibra changed the title [WIP] Making the module holonomic usable Making the module holonomic usable Jul 5, 2016
@shubhamtibra
Copy link
Contributor Author

Ping @certik @jksuom .

@@ -278,6 +278,10 @@ def unify(K0, K1, symbols=None):
else:
cls = K1.__class__

from sympy.polys.domains.old_polynomialring import GlobalPolynomialRing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if for speed reasons, you should rather be importing this at the top of the module? I think importing each time can be slow.

@certik
Copy link
Member

certik commented Jul 6, 2016

I think that this looks great. Any objections @jksuom ?

assert p == sqrt(x)
p = expr_to_holonomic(sqrt(1 + x**2)).to_expr()
assert p == sqrt(1+x**2)
p = expr_to_holonomic((2*x**2 + 1)**(2/3)).to_expr()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should use S(2)/3 here, otherwise you get a floating point number. Floating point numbers can be handled differently, either now or in the future, so the test could fail, but S(2)/3 should always work and be robust.

@jksuom
Copy link
Member

jksuom commented Jul 7, 2016

I agree that floating point numbers should be avoided in the tests unless they are the very subject of the test.. I think this is ready to be merged after that is fixed.

@shubhamtibra
Copy link
Contributor Author

Importing GlobalPolynomialRing at the top in sympy/polys/domains/domain.py is causing this error:

>>> bin/test holonomic
Traceback (most recent call last):
  File "bin/test", line 105, in <module>
    import sympy
  File "/home/shubham/sympy/sympy/__init__.py", line 49, in <module>
    from .polys import *
  File "/home/shubham/sympy/sympy/polys/__init__.py", line 5, in <module>
    from . import polytools
  File "/home/shubham/sympy/sympy/polys/polytools.py", line 54, in <module>
    from sympy.polys.domains import FF, QQ, ZZ
  File "/home/shubham/sympy/sympy/polys/domains/__init__.py", line 5, in <module>
    from . import domain
  File "/home/shubham/sympy/sympy/polys/domains/domain.py", line 13, in <module>
    from sympy.polys.domains.old_polynomialring import GlobalPolynomialRing
  File "/home/shubham/sympy/sympy/polys/domains/old_polynomialring.py", line 5, in <module>
    from sympy.polys.domains.ring import Ring
  File "/home/shubham/sympy/sympy/polys/domains/ring.py", line 5, in <module>
    from sympy.polys.domains.domain import Domain
ImportError: cannot import name Domain

Seems like Domain needs to be imported before GlobalPolynomialRing.

@jksuom
Copy link
Member

jksuom commented Jul 7, 2016

Seems like Domain needs to be imported before GlobalPolynomialRing.

That seems a good solution (if no other circular imports appear).

@shubhamtibra
Copy link
Contributor Author

shubhamtibra commented Jul 7, 2016

But importing GlobalPolynomialRing at the top in sympy/polys/domains/domain.py will still cause an error. Apparently, one cannot import GlobalPolynomialRing until the importing of Domain is done. So the statement from sympy.polys.domains.old_polynomialring import GlobalPolynomialRing should be inside the class. I can't think of an other solution.

@jksuom
Copy link
Member

jksuom commented Jul 7, 2016

I also think that must be done so. This is not the only case where importing in the class seems to be necessary.

@jksuom
Copy link
Member

jksuom commented Jul 8, 2016

I think this is ready. Merging.

@jksuom jksuom merged commit b0ad7ad into sympy:master Jul 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants