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

Introduced few query methods: is_Add, is_Mul & is_Pow #2351

Closed
wants to merge 3 commits into from

Conversation

anutosh491
Copy link
Collaborator

These are some query methods to check for the type of the expression. Couple other methods like is_Log and is_Exp would be introduced through subsequent PRs.

@certik
Copy link
Contributor

certik commented Sep 30, 2023

The PR is good. Can you add tests for this?

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Oct 3, 2023

Yeah I have raised 2 Prs for introducing query methods through SymEngine's Cwrapper

  1. Exposed get_base and get_exp functions symengine/symengine#1980
  2. Exposed some query methods symengine/symengine#1981

Once we have these I think we get most of the query methods covered !
Then we just need to add these to the process_attributes function in our pass to add support for these query methods.

@certik
Copy link
Contributor

certik commented Oct 3, 2023

I see, this PR was assuming some functionality in SymEngine that doesn't exist yet. I think we should rework it to do the changes ourselves. I think the basic_get_type(s) == SYMENGINE_ADD approach should be possible with what we have. The basic_get_args should also work hopefully.

@anutosh491
Copy link
Collaborator Author

Yes let me give it a try and get back to you !

@anutosh491
Copy link
Collaborator Author

Well basic_get_type returns a TypeID

TypeID basic_get_type(const basic s)
{
    return static_cast<TypeID>(s->m->get_type_code());
}

A TypeID is nothing but an enumeration registered in symengine/type_codes.inc

SYMENGINE_ENUM(SYMENGINE_INTEGER, Integer)
SYMENGINE_ENUM(SYMENGINE_RATIONAL, Rational)
SYMENGINE_ENUM(SYMENGINE_COMPLEX, Complex)
SYMENGINE_ENUM(SYMENGINE_COMPLEX_DOUBLE, ComplexDouble)

Well we could obviously introduce a similar enumeration but if we aren't looking forward to having query methods for each class, we just need to know the numbers (integers) which the Add, Mul, Pow and a couple other classes hold.
Our ccall could then look like

@ccall(header="symengine/cwrapper.h", c_shared_lib="symengine", c_shared_lib_path=f"{os.environ['CONDA_PREFIX']}/lib")
def basic_get_type(x: CPtr) -> i32:
    pass

And we can just compare the integer returned with the expected integer for the class (eg 17 for Pow).

@anutosh491
Copy link
Collaborator Author

I've just used up the enum values as of now. Maybe we could replicate the enumeration from SymEngine if we want to add more class types. Also we can currently support

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py 
from lpython import S
from sympy import Symbol, pi

def main0():
    x: S = Symbol("x")
    y: S = x**pi
    z1: bool = y.is_Pow
    print(z1)
    z2: bool = y.is_Add
    print(z2)
    z3: bool = y.is_Mul
    print(z3)

if __name__ == "__main__":
   main0()
(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --enable-symengine examples/expr2.py 
True
False
False

But if we would like to support cases like

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py 
from lpython import S
from sympy import Symbol, pi

def main0():
    x: S = Symbol("x")
    z1: bool = (x**pi).is_Pow
    print(z1)
    z2: bool = (x + pi).is_Add
    print(z2)
    z3: bool = (x*pi).is_Mul
    print(z3)

We would have to edit visit_Attribute in the python_ast_to_asr.cpp file, with an approach similar to #2337 but then that would end up polluting the frontend with SymPy specific features. Maybe I could introduce that through a subsequent pull request.

@anutosh491
Copy link
Collaborator Author

I'll add a simple test for our implementation.

ASRUtils::TYPE(ASR::make_Integer_t(al, loc, 4)), nullptr, nullptr));
// Using 16 as the right value of the IntegerCompare node as it represents SYMENGINE_ADD through SYMENGINE_ENUM
return ASRUtils::EXPR(ASR::make_IntegerCompare_t(al, loc, function_call, ASR::cmpopType::Eq,
ASRUtils::EXPR(ASR::make_IntegerConstant_t(al, loc, 16, ASRUtils::TYPE(ASR::make_Integer_t(al, loc, 4)))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a variable / macro and define these all at one place in this file. They will be dependent on the SymEngine version, but I think that's fine. If this becomes a problem, we can revisit the SymEngine API then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. I'll look into this!

Copy link

Choose a reason for hiding this comment

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

You can use basic_get_class_id("Integer") to get the value.

@certik
Copy link
Contributor

certik commented Oct 3, 2023

SymEngine.py supports both:

In [8]: isinstance(x**2, se.Pow)
Out[8]: True

In [9]: (x**2).is_Pow
Out[9]: True

Is supporting isinstance(x**2, se.Pow) easier in the frontend?

I think just do (x**2).is_Pow for now. I think this is a common feature to have a custom type (Symbolic in this case) and be able to support custom methods and attributes on it. So we need to refactor our frontend to support this use case better.

@anutosh491
Copy link
Collaborator Author

I think we should be able to do handle isinstance in the frontend. We anyways need to introduce cases like isinstance(log(x), log) as we lack a method like is_Log. Hence I shall be handling it in the subsequent pr

@certik
Copy link
Contributor

certik commented Oct 3, 2023

Regarding supporting is_Pow at all, I created a dedicated issue for that: #2360.

@anutosh491
Copy link
Collaborator Author

Thanks for raising the issue up, I'm still not fully sure what's the best approach here but I'll try to introduce the .func attribute through a separate branch and we could make this pr as draft untill then.

@anutosh491 anutosh491 marked this pull request as draft October 4, 2023 06:16
@anutosh491
Copy link
Collaborator Author

anutosh491 commented Oct 4, 2023

Well I think so the Pow here is just the class type which we would have to import if we want to compare it through the .func method
.So the following would get caught

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py 
from lpython import S
from sympy import Symbol, pi

def main0():
    x: S = Symbol("x")
    y: S = x**pi
    print(y.func == Pow)
    
(lf) anutosh491@spbhat68:~/lpython/lpython$ PYTHONPATH="src/runtime/lpython" python examples/expr2.py
Traceback (most recent call last):
  File "/home/anutosh491/lpython/lpython/examples/expr2.py", line 10, in <module>
    main0()
  File "/home/anutosh491/lpython/lpython/examples/expr2.py", line 7, in main0
    print(y.func == Pow)
                    ^^^
NameError: name 'Pow' is not defined. Did you mean: 'pow'?
(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py 
from lpython import S
from sympy import Symbol, pi, Pow

def main0():
    x: S = Symbol("x")
    y: S = x**pi
    print(y.func == Pow)

(lf) anutosh491@spbhat68:~/lpython/lpython$ PYTHONPATH="src/runtime/lpython" python examples/expr2.py
True

So we essentially would like to return a class type through the .func call

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py 
from lpython import S
from sympy import Symbol, pi

def main0():
    x: S = Symbol("x")
    y: S = x**pi
    print(y.func)

if __name__ == "__main__":
   main0()
(lf) anutosh491@spbhat68:~/lpython/lpython$ PYTHONPATH="src/runtime/lpython" python examples/expr2.py
<class 'sympy.core.power.Pow'>

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Oct 4, 2023

I am not sure what is the significance of what we are returning (depends on the use case). We surely won't face any confusion like this through a query method like is_XX or using isinstance. I think the .func attribute is more than just a query method

  1. We can simply return the String in consideration here . For eg
def main0():
    _x: i64 = i64(0)
    x: CPtr = empty_c_void_p()
    p_c_pointer(pointer(_x, i64), x)
    basic_new_stack(x)
    basic_const_pi(x)
    _y: i64 = i64(0)
    y: CPtr = empty_c_void_p()
    p_c_pointer(pointer(_y, i64), y)
    basic_new_stack(y)
    symbol_set(y, "y")
    basic_pow(y, y, x)
    # Here we could have a switch case or something to check type of y against
    # various types (passed as strings) and if something matches we can just return the string
    print(basic_get_type(y) == basic_get_class_id("Pow"))
    
(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --enable-symengine symbolics.py
True
  1. If we plan on returning classes we would like to introduce subclasses/child type for the SymbolicExpression ttype (parent type) like SymbolicPow etc. Once we have a ttype we could use the make_Class_t function to return a class.

@certik
Copy link
Contributor

certik commented Oct 4, 2023

You can use the basic_get_type(y) == basic_get_class_id("Pow") approach for now, that should be enough to get started, but it's not going to be the fastest. Why not using the direct numbers as you did? I think that will be the fastest.

@certik
Copy link
Contributor

certik commented Oct 4, 2023

I asked about the stability of the type ordering here: symengine/symengine#1982

@anutosh491
Copy link
Collaborator Author

Query methods for class Add, Mul and Pow were implemented through #2367, hence this can be closed !

@anutosh491 anutosh491 closed this Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants