-
Notifications
You must be signed in to change notification settings - Fork 67
Miscellaneous Additions #179
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
Conversation
symengine/lib/symengine_wrapper.pyx
Outdated
@@ -2977,11 +2979,16 @@ zoo = c2py(symengine.ComplexInf) | |||
nan = c2py(symengine.Nan) | |||
true = c2py(symengine.boolTrue) | |||
false = c2py(symengine.boolFalse) | |||
try: | |||
import sympy | |||
hbar = PyFunction(sympy.hbar, (), sympy.hbar.func, sympy_module) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@isuruf This seemingly doesn't work as it returns:
'module' object has no attribute 'hbar'
I tried changing this to:
PyFunction(sympy.physics.quantum.constants.HBar, (), sympy.physics.quantum.constants.HBar.func, sympy_module)
but even that returns:
'module' object has no attribute 'physics'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @isuruf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is hbar
imported in sympy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Through sympy.physics.quantum.constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean the exact statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you try that then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment here. Changing to hbar
makes no difference as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean from sympy.physics.quantum.constants import hbar
or sympy.hbar
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This:
PyFunction(sympy.physics.quantum.constants.hbar, (), sympy.physics.quantum.constants.hbar.func, sympy_module)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant this,
from sympy.physics.quantum.constants import hbar as hbar_sympy
PyFunction(hbar_sympy, (), hbar_sympy, sympy_module)
symengine/lib/symengine_wrapper.pyx
Outdated
@@ -2977,11 +3029,16 @@ zoo = c2py(symengine.ComplexInf) | |||
nan = c2py(symengine.Nan) | |||
true = c2py(symengine.boolTrue) | |||
false = c2py(symengine.boolFalse) | |||
try: | |||
import sympy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds sympy's initialization time to symengine's initialization time. That should be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you going to use hbar
in sympy? sympy.physics
would import from sympy.core.backend which would in turn import from sympy.physics
for hbar
if USE_SYMENGINE=1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, in certain places there's code like this:
return exp(-I*self.position*bra.momentum/hbar)/sqrt(2*pi*hbar)
which gives the error:
Cannot convert `hbar` to a symengine type.
in sympy.physics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Say you define hbar
in symengine. What changes are you going to do in sympy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If hbar
as an object under class Constant
is defined in SymEngine
, I'd follow the same routine that we follow for all the other objects. That's why I asked for implementing the same in the Gitter
channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not the same as others. hbar
is not defined in sympy.core
, but in sympy.physics
.
sympy.physics
would import from sympy.core.backend
which would in turn import from sympy.physics
for hbar
if USE_SYMENGINE=1
leading to a cyclic dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to define hbar
in sympy.physics
using PyFunction
? That way I think, we could have a check for USE_SYMENGINE=1
in the respective modules and define hbar
if the condition is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymPy developers are of the opinion that only imports should change and USE_SYMENGINE=1
should be used in sympy.core.backend
only.
For this case, I think you'd have to write a new class NumberSymbol
that is a child class of Constant
and do something like what NumberWrapper
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do that in another PR. Thanks for clarifying my doubts.
symengine/lib/symengine_wrapper.pyx
Outdated
def copy(self): | ||
return self.func(*self.args) | ||
|
||
def xreplace(self, rule): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already defined.
symengine/lib/symengine_wrapper.pyx
Outdated
value, _ = self._xreplace(rule) | ||
return value | ||
|
||
def _xreplace(self, rule): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is slow. Use subs
to do this and check that the result is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cross checked again, this isn't needed at all in the latest master of SymPy
.
a96221b
to
a57676e
Compare
@isuruf The implementation work here is done. Can you review? I'll add the attributes and |
symengine/lib/symengine_wrapper.pyx
Outdated
class AssocOp(Basic): | ||
|
||
@classmethod | ||
def _from_args(cls, args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is untested.
symengine/lib/symengine_wrapper.pyx
Outdated
@@ -689,7 +689,7 @@ cdef class Basic(object): | |||
elif (op == 5): | |||
return c2py(<RCP[const symengine.Basic]>(symengine.Ge(A.thisptr, B.thisptr))) | |||
|
|||
def expand(Basic self not None): | |||
def expand(Basic self not None, *args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this ignore parameters is not good. We should implement them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual issue that I was facing was that during the use of exp
in SymPy
, the associated object was making a call like:
e.expand(complex=True)
I don't think the expansion for complex domain has been implemented in SymEngine
for now. What do you suggest we do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @isuruf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned we should implement them. Accepting any argument and doing nothing is worse than throwing an error, because it may suggest to the user that it is allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not find how SymPy
handles complex expansion. I tried introducing keywords in Pow
and the trigonometric functions, but the parser is throwing up issues. I think it would be better if these were handled later, as it would take up a lot of time right now. I'm dropping the commit. These are required only for a single import in SymPy
.
4d31d7a
to
1c0a50c
Compare
@isuruf Can you help me in fixing the current error log here as well? |
symengine/lib/symengine_wrapper.pyx
Outdated
@@ -825,6 +825,9 @@ cdef class Basic(object): | |||
def is_Matrix(self): | |||
return False | |||
|
|||
def copy(self): | |||
return self.__class__(*self.args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the usage of copy is since this object is immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This directly follows from SymPy
's implementation of Basic
. Should I remove it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just return self
87e8559
to
a25dfbe
Compare
@sumith1896 @certik Can you review and merge this please? |
I'm having a look |
symengine/lib/symengine_wrapper.pyx
Outdated
@@ -787,6 +787,30 @@ cdef class Basic(object): | |||
|
|||
evalf = n | |||
|
|||
def matches(self, expr, repl_dict={}, old=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please implement this in symengine, not symengine.py
@isuruf I've taken a note of the changes for further improvement. Can you review and merge this? We'll need to have a release in |
We'll use a dev commit for SymPy testing. I don't want to make a release in a rush |
Ok, so can I have a review of this? |
No description provided.