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

scipy in Lambdify #7229 #10319

Closed
wants to merge 13 commits into from
Closed

Conversation

Shekharrajak
Copy link
Member

7229

  • Addition of namespaces ,namespaces_default,translation for scipy in lambdify.py
  • added tests in tests/test_lambdify.py (some of them are from previous old PRs)
  • updated importtools.py for scipy
  • and some basic modifications.

@Shekharrajak Shekharrajak changed the title scipy in Lambdify [#7229](https://github.com/sympy/sympy/issues/7229) scipy in Lambdify #7229 Dec 26, 2015
from sympy.functions.special.hyper import *
from sympy.functions.special.hyper import *
from sympy.functions.special.gamma_functions import *
from sympy.functions.special.gamma_functions import *
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use the from x import * style of import. Always make it explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@Shekharrajak
Copy link
Member Author

Why this error :

func0 = lambdify((), expr, modules="mpmath", printer=intervalrepr)
NameError: global name 'intervalrepr' is not defined

line

@moorepants
Copy link
Member

Do you have it defined? Please post your whole code snippet that you are running? Or are you running the test?

@Shekharrajak
Copy link
Member Author

@moorepants , During running the tests in Travis that error is occurring.I did not define,that line is already there.

@@ -24,12 +24,12 @@ def eq(a, b, tol=1e-6):

def test_jn_zeros():
assert eq(jn_zeros(0, 4, method="scipy"),
[3.141592, 6.283185, 9.424777, 12.566370])
[3.141592, 6.283185, 9.424777, 12.566370]) == True
Copy link
Member

Choose a reason for hiding this comment

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

Why are these changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

test cases returns True so I think it will be better if we write it clearly., what is the output.

Copy link
Member

Choose a reason for hiding this comment

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

The convention (see PEP8) is to not compare to True like this. It is unnecessary. Please revert.

#Try if you can extract symbols from the expression.
#Move on if expr.atoms in not implemented.
# Try if you can extract symbols from the expression.
# Move on if expr.atoms in not implemented.
Copy link
Member

Choose a reason for hiding this comment

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

Please keep all of these kinds of fixes in a separate PR (or at least separate commit). I makes it difficult to focus on the actual code changes.

@moorepants
Copy link
Member

All of the formatting changes make it difficult to see the code changes. It is much more preferable and easier for the reviewers if you do formatting in a different PR.

@moorepants moorepants added utilities.lambdify PR: author's turn The PR has been reviewed and the author needs to submit more changes. labels Jan 1, 2016
@Shekharrajak Shekharrajak force-pushed the working#7229 branch 2 times, most recently from 3c64b54 to 729909d Compare January 8, 2016 17:00
@moorepants
Copy link
Member

@Shekharrajak
Copy link
Member Author

Check this modification
Should it be like this ? I don't find proper example to use numpy.allclose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: author's turn The PR has been reviewed and the author needs to submit more changes. utilities.lambdify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants