Wrapped Min and Max Functions #126

ShikharJ · 2017-02-25T08:07:46Z

@isuruf Can you tell me why are the current builds failing? Everything seems to be in compliance.

isuruf · 2017-02-25T08:11:57Z

See this error message,

Error compiling Cython file:
------------------------------------------------------------
...
    cdef symengine.vec_basic v
    cdef Basic e_
    for e in args:
        e_ = sympify(e)
        v.push_back(e_.thisptr)
    return c2py(symengine.max(v))
                              ^
------------------------------------------------------------
symengine_wrapper.pyx:2250:31: Cannot assign type 'vec_basic' to 'RCP[const Basic]'

ShikharJ · 2017-02-25T08:14:24Z

I mean, isn't symengine.max supposed to take a vec_basic, or do I have to run a loop for individual elements?

isuruf · 2017-02-25T09:33:31Z

symengine/lib/symengine.pxd

@@ -417,6 +419,7 @@ cdef extern from "<symengine/functions.h>" namespace "SymEngine":
    cdef RCP[const Basic] acoth(RCP[const Basic] &arg) nogil except+
    cdef RCP[const Basic] function_symbol(string name, const vec_basic &arg) nogil except+
    cdef RCP[const Basic] abs(RCP[const Basic] &arg) nogil except+
+    cdef RCP[const Basic] max(RCP[const Basic] &arg) nogil except+


This function signature is wrong

ShikharJ · 2017-02-25T18:32:18Z

I think this is ready for review now @isuruf.

ShikharJ · 2017-02-27T12:38:05Z

ping @isuruf

isuruf · 2017-02-28T16:33:29Z

symengine/tests/test_functions.py

+    assert max({Integer(6)/3, 1}) == 2
+    assert max({-2, 2}) == 2
+    assert max({2, 2}) == 2
+    assert max({0.2, 0.3}) == 0.3


This is using python's min and max.

isuruf · 2017-02-28T16:36:05Z

symengine/lib/symengine_wrapper.pyx

+        v.push_back(e_.thisptr)
+    return c2py(symengine.max(v))
+
+def min(*args):


It's not good to change a function in the global namespace of python. What does sympy do?

ShikharJ · 2017-03-02T10:15:11Z

@isuruf From what I observe, Sympy has a Max and Min class defined which inherits from MaxMinBase, which itself inherits from Expr and LatticeOp. Individual max and min functions are defined, though only as member functions, that too of some specific classes.

ShikharJ · 2017-03-04T04:48:54Z

@isuruf What do you suggest we do of the namespace issue?

isuruf · 2017-03-04T04:53:03Z

Same as in SymPy. We define the classes Max and Min only.

ShikharJ · 2017-03-04T09:57:42Z

@isuruf I'am facing trouble implementing that. Min and Max both have constructors of the order Class::Class(const vec_basic &&args) which is raising issues in the builds. Can you please take a look and suggest improvements?

ShikharJ · 2017-03-06T05:58:29Z

Ping @isuruf

isuruf · 2017-03-06T08:19:21Z

Use min and max, don't use the constructors of Max and Min. Constructors of Max and Min are supposed to be used only when you know the result is a Max or a Min. You don't want to create a Max(1, 2)

ShikharJ · 2017-03-06T11:42:07Z

@isuruf I'm not sure what you mean. Wouldn't using max and min bring us back to the same problem of namespace?

ShikharJ · 2017-03-07T06:41:41Z

Ping @isuruf

isuruf · 2017-03-07T10:08:26Z

symengine/tests/test_functions.py

@@ -1,5 +1,5 @@
 from symengine import Symbol, sin, cos, sqrt, Add, Mul, function_symbol, Integer, log, E, symbols
-from symengine.lib.symengine_wrapper import Subs, Derivative
+from symengine.lib.symengine_wrapper import Subs, Derivative, _max, _min


We don't want _max to be the public interface.
Max can be the public interface, but for that we need to rename the existing class to _Max and make Max, a pure python class and override __new__ to return the value, like in https://github.com/symengine/symengine.py/blob/master/symengine/sympy_compat.py#L53-L54

Thanks for clearing me up on this.

ShikharJ · 2017-03-08T13:44:57Z

@isuruf Can you please take a look now?

isuruf

Looks good to me.
Only issue is that Max can only be imported from sympy_compat. We should be able to use it from symengine_wrapper as well.

ShikharJ · 2017-03-27T14:20:27Z

@isuruf I have added a string to address the import issues. I'd like to raise an issue for a possible workaround for the above. Can you please take a look here?

ShikharJ · 2017-03-29T15:15:16Z

Ping @isuruf

isuruf · 2017-03-29T15:26:36Z

Shifting the tests to sympy_compat is not a real solution. we need to give the user a way to use Max and Min when they do from symengine import *

ShikharJ · 2017-03-29T17:36:54Z

@isuruf Shifting the tests was never intended as a solution to the import issue. It was more along the lines of current necessity. As I'm out of ideas regarding the solution of the namespace issue, I would like to raise an issue, for possible ideas regarding the same, after this is merged. I pinged for a final review of this code.

…ympy_compat.py

ShikharJ · 2017-06-01T12:20:01Z

+1

ShikharJ · 2017-06-01T16:22:29Z

Thanks for guiding me through @isuruf.

isuruf reviewed Feb 25, 2017

View reviewed changes

ShikharJ force-pushed the MinMax branch from 61e4c2b to 4faa340 Compare February 25, 2017 10:15

ShikharJ changed the title ~~[WIP] Wrapped Min and Max Functions~~ Wrapped Min and Max Functions Feb 25, 2017

isuruf requested changes Feb 28, 2017

View reviewed changes

ShikharJ force-pushed the MinMax branch from 8827477 to 5d469a3 Compare March 3, 2017 20:12

ShikharJ force-pushed the MinMax branch from 27f19ab to 055ea0d Compare March 6, 2017 16:19

isuruf reviewed Mar 7, 2017

View reviewed changes

isuruf reviewed Mar 8, 2017

View reviewed changes

ShikharJ force-pushed the MinMax branch 3 times, most recently from 24f105a to da6e5e0 Compare March 25, 2017 05:56

isuruf modified the milestone: 0.3.0 May 11, 2017

ShikharJ added 2 commits May 15, 2017 16:38

Wrapped Max() Function

c643d39

Wrapped Min() Function

032528d

ShikharJ force-pushed the MinMax branch from da6e5e0 to ca4d59a Compare May 15, 2017 11:48

Introduced Min and Max in sympy_compat.py and shifted tests to test_s…

7ce3b5a

…ympy_compat.py

ShikharJ force-pushed the MinMax branch from ca4d59a to 7ce3b5a Compare May 15, 2017 13:19

Fix Max and Min

cb5be46

isuruf merged commit e3f4bd2 into symengine:master Jun 1, 2017

ShikharJ deleted the MinMax branch June 1, 2017 16:22

isuruf mentioned this pull request Jun 3, 2017

Wrap min and max function #54

Closed

Wrapped Min and Max Functions #126

Wrapped Min and Max Functions #126

Uh oh!

Conversation

ShikharJ commented Feb 25, 2017

Uh oh!

isuruf commented Feb 25, 2017

Uh oh!

ShikharJ commented Feb 25, 2017

Uh oh!

isuruf Feb 25, 2017

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShikharJ commented Feb 27, 2017

Uh oh!

isuruf Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

isuruf Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Mar 2, 2017

Uh oh!

ShikharJ commented Mar 4, 2017

Uh oh!

isuruf commented Mar 4, 2017

Uh oh!

ShikharJ commented Mar 4, 2017

Uh oh!

ShikharJ commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isuruf commented Mar 6, 2017

Uh oh!

ShikharJ commented Mar 6, 2017

Uh oh!

ShikharJ commented Mar 7, 2017

Uh oh!

isuruf Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

ShikharJ Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Mar 8, 2017

Uh oh!

isuruf left a comment

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Mar 27, 2017

Uh oh!

ShikharJ commented Mar 29, 2017

Uh oh!

isuruf commented Mar 29, 2017

Uh oh!

ShikharJ commented Mar 29, 2017

Uh oh!

ShikharJ commented Jun 1, 2017

Uh oh!

ShikharJ commented Jun 1, 2017

Uh oh!

Uh oh!

ShikharJ commented Feb 25, 2017 •

edited

Loading

ShikharJ commented Mar 6, 2017 •

edited

Loading