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.

Sign up for GitHub

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

Jump to bottom

Logic Classes and Sets #168

Merged
merged 6 commits into from
Aug 10, 2017
Merged

Logic Classes and Sets #168

merged 6 commits into from
Aug 10, 2017

Conversation

ShikharJ
Copy link
Member

No description provided.

ShikharJ
ShikharJ commented Jun 29, 2017
View reviewed changes
symengine/lib/symengine_wrapper.pyx Outdated
cdef Boolean e_
for e in args:
e_ = sympify(e)
s.insert(e_.thisptr)
Copy link
Member Author

Choose a reason for hiding this comment

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

@isuruf Why isn't insert identified as a part of set_boolean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping @isuruf.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by that?

Copy link
Member Author

Choose a reason for hiding this comment

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

The builds fail because of the following error:

 Cythonizing symengine_wrapper.pyx
  
  Error compiling Cython file:
  ------------------------------------------------------------
  ...
  def logical_and(*args):
      cdef symengine.set_boolean s
      cdef Boolean e_
      for e in args:
          e_ = sympify(e)
          s.insert(e_.thisptr)
          ^
  ------------------------------------------------------------
  
  C:\projects\symengine-py-l1jmr\symengine\lib\symengine_wrapper.pyx:3043:9: Object of type 'set_boolean' has no attribute 'insert'

Copy link
Member

Choose a reason for hiding this comment

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

Since it says it is not defined, you have to define it in the pxd file

@ShikharJ ShikharJ force-pushed the Logic branch 3 times, most recently from 14e0256 to 8b1e426 Compare June 30, 2017 23:00
@ShikharJ ShikharJ changed the title [WIP] Logic Classes Logic Classes and Sets Jun 30, 2017
@ShikharJ ShikharJ force-pushed the Logic branch 2 times, most recently from 4c0f4f2 to 352be01 Compare July 1, 2017 19:54
@ShikharJ
Copy link
Member Author

ShikharJ commented Jul 2, 2017

@isuruf I'm out of ideas on fixing the current build failure. Can you please take a look, and suggest what can be done?

@isuruf
Copy link
Member

isuruf commented Jul 3, 2017

Here is the relevant line,

symengine.py/symengine/lib/symengine.pxd

Line 43 in b409536

iterator end() nogil

Similar line,

symengine.py/symengine/lib/symengine.pxd

Line 54 in b409536

iterator insert(T&) nogil

@ShikharJ
Copy link
Member Author

ShikharJ commented Jul 3, 2017

@isuruf Those changes have already been applied. I was referring to the current build failure (the output was too big to post here), which you can view in the Appveyor builds.

@ShikharJ ShikharJ force-pushed the Logic branch 2 times, most recently from 17eca44 to 11b40df Compare July 4, 2017 12:14
isuruf
isuruf reviewed Jul 4, 2017
View reviewed changes
symengine/lib/symengine.pxd Outdated
@@ -203,7 +206,7 @@ cdef extern from "<symengine/basic.h>" namespace "SymEngine":
ctypedef map[RCP[Integer], unsigned] map_integer_uint "SymEngine::map_integer_uint"
cdef struct RCPIntegerKeyLess
cdef struct RCPBasicKeyLess
ctypedef set[RCP[const_Basic], RCPBasicKeyLess] set_basic "SymEngine::set_basic"
ctypedef set[RCP[Basic], RCPBasicKeyLess] set_basic "SymEngine::set_basic"
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? Might be the issue

Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting this error without the above change:

Cythonizing symengine_wrapper.pyx
  
  Error compiling Cython file:
  ------------------------------------------------------------
  ...
  def finiteset(*args):
      cdef symengine.set_basic s
      cdef Basic e_
      for e in args:
          e_ = sympify(e)
          s.insert(e_.thisptr)
                    ^
  ------------------------------------------------------------
  
  C:\projects\symengine-py\symengine\lib\symengine_wrapper.pyx:3922:19: Cannot assign type 'RCP[const Basic]' to 'RCP[const_Basic]'

Copy link
Member

Choose a reason for hiding this comment

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

Cast e_.thisptr to RCP[const_Basic] instead of the above change.

@ShikharJ ShikharJ force-pushed the Logic branch 2 times, most recently from e7db1df to 58dbfa5 Compare July 11, 2017 18:32
@ShikharJ ShikharJ force-pushed the Logic branch 2 times, most recently from fca3ca6 to 1326a2d Compare July 16, 2017 15:55
@ShikharJ
Copy link
Member Author

ShikharJ commented Aug 3, 2017

@isuruf Can you help me in getting this sorted out?

ShikharJ
ShikharJ commented Aug 3, 2017
View reviewed changes
symengine/lib/symengine_wrapper.pyx
cdef Basic e_
for e in args:
e_ = sympify(e)
s.insert(<RCP[symengine.const_Basic]>(e_.thisptr))
Copy link
Member Author

Choose a reason for hiding this comment

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

@isuruf This is still failing.

Copy link
Member

Choose a reason for hiding this comment

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

The failure now is not because of that. Open symengine_wrapper.cpp and see which line from symengine_wrapper.pyx is causing the problem. You can find the line by scrolling above and looking for the first comment.

Copy link
Member

Choose a reason for hiding this comment

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

@ShikharJ, what did you get?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was referring to the errors in the current log. This is fixed. The issues are only arising while running tests.

@ShikharJ
Copy link
Member Author

ShikharJ commented Aug 4, 2017

Ping @isuruf.

@ShikharJ ShikharJ force-pushed the Logic branch 4 times, most recently from 5c3a9d8 to ea09ced Compare August 8, 2017 14:45
@ShikharJ
Copy link
Member Author

ShikharJ commented Aug 8, 2017

@isuruf Can you help me in fixing the errors?

isuruf
isuruf reviewed Aug 9, 2017
View reviewed changes
symengine/lib/symengine_wrapper.pyx Outdated
@@ -182,6 +182,34 @@ cdef c2py(RCP[const symengine.Basic] o):
r = Function.__new__(conjugate)
elif (symengine.is_a_PyNumber(deref(o))):
r = PyNumber.__new__(PyNumber)
elif (symengine.is_a_Piecewise(deref(o))):
r = Piecewise.__new__(Piecewise)
Copy link
Member

Choose a reason for hiding this comment

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

Use Basic.__new__(Piecewise) or otherwise you get a infinite cycle.

@ShikharJ ShikharJ force-pushed the Logic branch 2 times, most recently from 88555d8 to 9a4e1f9 Compare August 9, 2017 11:02
@ShikharJ
Copy link
Member Author

ShikharJ commented Aug 9, 2017

This is ready for a review @isuruf.

isuruf
isuruf reviewed Aug 9, 2017
View reviewed changes
symengine/lib/symengine_wrapper.pyx
l = []
for i in range(0, len(a), 2):
l.append((a[i]._sympy_(), a[i + 1]._sympy_()))
return sympy.Piecewise(*l)
Copy link
Member

Choose a reason for hiding this comment

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

This is not tested.

isuruf
isuruf reviewed Aug 9, 2017
View reviewed changes
symengine/tests/test_solve.py Outdated
@@ -0,0 +1,17 @@
from symengine.utilities import raises
from symengine.lib.symengine_wrapper import (Interval, EmptySet, FiniteSet,
I, oo, solve, Eq, Symbol)
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the spacing here?

isuruf
isuruf reviewed Aug 9, 2017
View reviewed changes
symengine/tests/test_sets.py Outdated
from symengine.utilities import raises
from symengine.lib.symengine_wrapper import (Interval, EmptySet, UniversalSet, FiniteSet,
Union, Complement, ImageSet, ConditionSet,
And, Or, oo, Symbol, true, Ge, Eq, Gt)
Copy link
Member

Choose a reason for hiding this comment

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

Fix formatting here.

isuruf
isuruf reviewed Aug 9, 2017
View reviewed changes
symengine/tests/test_sets.py Outdated
assert Interval(1, 1, True, True) == EmptySet()
assert Interval(1, 2).union(Interval(2, 3)) == Interval(1, 3)


Copy link
Member

Choose a reason for hiding this comment

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

Remove whitespace here

@ShikharJ
Copy link
Member Author

Updated @isuruf.

@isuruf isuruf merged commit 445559e into symengine:master Aug 10, 2017
@ShikharJ ShikharJ deleted the Logic branch August 10, 2017 11:25
@ShikharJ
Copy link
Member Author

Thanks for the help @isuruf.

@isuruf isuruf mentioned this pull request Aug 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isuruf isuruf isuruf left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ShikharJ @isuruf