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

Intial Implementation for SymbolicExpression ttype #1846

Merged
merged 40 commits into from
Jun 19, 2023

Conversation

anutosh491
Copy link
Collaborator

Through this Pr , I would like to solve a basic program involving symbolic expressions , which is

from lpython import S
from sympy import Symbol

def main0():
   x: S = Symbol('x')
   print(x)

main0()

This essentially involves few steps

  1. Supporting imports from SymPy for CPython
  2. introducing the SymbolicExpression ttype
  3. introducing SymbolicSymbol as an intrinsic function
  4. Framing a pass in intrinsic_function.cpp to make calls to SymEngine's C interface
  5. getting the backend (preferrably LLVM) to work .

Currently I've tried to address the 2nd and 3rd step !

@certik
Copy link
Contributor

certik commented May 22, 2023

See also #1591.

@certik
Copy link
Contributor

certik commented May 28, 2023

You must resolve conflicts so that the CI tests run.

@certik certik marked this pull request as draft May 31, 2023 03:28
@certik
Copy link
Contributor

certik commented May 31, 2023

I think you have to update reference results with ./run_tests -u and commit the changes.

@anutosh491
Copy link
Collaborator Author

Hello @certik . I've got the C code generated for a simple assignment program . Like the following

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py 
from sympy import Symbol
def main0():
    x: S = Symbol('x')

main0()

It looks like the following

(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --show-c examples/expr2.py 
#include <symengine/cwrapper.h>

#include <stdlib.h>
#include <stdbool.h>
#include <stdio.h>
#include <string.h>
#include <lfortran_intrinsics.h>


struct dimension_descriptor
{
    int32_t lower_bound, length;
};

// Implementations
basic _lcompilers_symbolic_str(char * x)
{
    basic _lcompilers_symbolic_str;
    _lcompilers_symbolic_str = _lfortran_set_symbol(x);
    return _lcompilers_symbolic_str;
}

void main0()
{
    basic x;
    x = _lcompilers_symbolic_str("x");
}

void _lpython_main_program()
{
    main0();
}

int main(int argc, char* argv[])
{
    _lpython_set_argv(argc, argv);
    _lpython_main_program();
    return 0;
}

where _lfortran_set_symbol would be defined in lfortran_intrinsic.c and would be using the symbol_set function from symengine/cwrapper.h (somethine like symbol_set(x, 'x') ) . Does this approch look good to you ?
I'll be ready with the C generated code for print statement too !

@certik
Copy link
Contributor

certik commented Jun 12, 2023

Yes, that looks good. As we discussed, I would "inline" the _lcompilers_symbolic_str function.

@anutosh491
Copy link
Collaborator Author

Yes, that looks good. As we discussed, I would "inline" the _lcompilers_symbolic_str function.

Yes . I will address that in the following commit (just refining the code). Currently I have the following

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py 
from sympy import Symbol
def main0():
    x: S = Symbol('x')
    y: S = Symbol('y')
    x = y
    z: S = x + y

main0()

(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --show-c examples/expr2.py 
#include <math.h>
#include <symengine/cwrapper.h>

#include <stdlib.h>
#include <stdbool.h>
#include <stdio.h>
#include <string.h>
#include <lfortran_intrinsics.h>


struct dimension_descriptor
{
    int32_t lower_bound, length;
};

// Implementations
void main0()
{
    basic x;
    basic_new_stack(x);
    basic y;
    basic_new_stack(y);
    basic z;
    basic_new_stack(z);
    symbol_set(x, "x");
    symbol_set(y, "y");
    basic_assign(x, y);
    basic_add(z, x, y);
}

void _lpython_main_program()
{
    main0();
}

int main(int argc, char* argv[])
{
    _lpython_set_argv(argc, argv);
    _lpython_main_program();
    return 0;
}

@certik
Copy link
Contributor

certik commented Jun 13, 2023

Make sure you enable -DWITH_RUNTIME_STACKTRACE=yes.

@certik
Copy link
Contributor

certik commented Jun 13, 2023

Very good, excellent progress!

I think now we just need to print it, so that we can see some result when we compile this.

@anutosh491
Copy link
Collaborator Author

I think now we just need to print it, so that we can see some result when we compile this.

Yes, I have been generating C code , then taking the main function out of it , I generate a c file followed by an o and an out file linked with symengine for printing the output !

Also my latest commit adds functionality for supporting symbolic constants , for now I've added pi . So if we have the following program

from sympy import Symbol, pi
def main0():
    x: S = Symbol('x')
    y: S = Symbol('y')
    x = pi
    z: S = x + y
    print(z)

main0()

The C code we generate for this looks like

// Implementations
void main0()
{
    basic x;
    basic_new_stack(x);
    basic y;
    basic_new_stack(y);
    basic z;
    basic_new_stack(z);
    symbol_set(x, "x");
    symbol_set(y, "y");
    basic_const_pi(x);
    basic_add(z, x, y);
    printf("%s\n", basic_str(z));
}

It gives me y + pi when I print it out !

@certik
Copy link
Contributor

certik commented Jun 14, 2023

Here is the first test:

from sympy import Symbol, pi
from lpython import S

def main0():
    x: S = Symbol('x')
    y: S = Symbol('y')
    x = pi
    z: S = x + y
    print(z)

main0()

TODO:

  • Move your local examples/expr2.py (the above test) to integration_test/symbolics_01.py
  • Enable it using C and CPython and make the CI pass
    • Probably lpython.py needs adding S (from lpython import S)
  • CI: add a dedicated new GitHub Action test for sympy; in there install sympy and symengine into the conda environment
  • in LPython, we need to add an option to CompilerOptions (compiler_options.enable_symengine) that symengine is enabled (default false); if enabled, add those extra -I and -L -lsymengine options to the --backend=c handling, otherwise not.
  • add --enable-symengine Enable symengine runtime
  • Remove the LLVM backend changes (we'll do those in separate PR)

@certik
Copy link
Contributor

certik commented Jun 18, 2023

Overall I think this PR looks pretty good. If @Shaikh-Ubaid is fine with it, I'll give it a thorough review and testing tomorrow and we can then merge it.

@anutosh491
Copy link
Collaborator Author

I do see some improvements which should be introduced in SymbolicAdd etc
But I'll keep it for the next Pr and not disturb this one . As far as we are able to add a symbol to pi or something , I'll keep this as it is.

@Shaikh-Ubaid
Copy link
Collaborator

@anutosh491 Please make a note of changes we discussed to be tackled later.

Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thank you so much for this @anutosh491 !

@anutosh491
Copy link
Collaborator Author

@anutosh491 Please make a note of changes we discussed to be tackled later.

Yes , I've made a note of the things that have been pointed out by you and @Thirumalai-Shaktivel . I'll address them in the upcoming Prs.

@anutosh491 anutosh491 marked this pull request as ready for review June 19, 2023 12:37
@Shaikh-Ubaid
Copy link
Collaborator

Also another minor point, it seems S is a common variable name. For example, it could be used as

class S:
...

s: S

or

S: Student // where Student is some class defined

It seems to (easily) conflict with the LPython annotation for Symbolic type. Is it possible/better to use another name for LPython Symbolic type?

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Jun 19, 2023

We actually stuck to S as sympy uses S to "sympify" things , which means it comes handy to convert lets say primitive variables to SymPy objects

>>> type(2)
<class 'int'>
>>> print(type(S(2))
<class 'sympy.core.numbers.Integer'>

So S essentially signifies that an object is compatible with SymPy or rather a Symbolic object !

But I get the fact that this is a bit vulnerable as shown by your examples above !

@certik
Copy link
Contributor

certik commented Jun 19, 2023

We need to eventually allow renaming, like from lpython import S as Symbolic or import lpython; lpython.S, which will resolve any ambiguities. I think S is fine, since it's short and compatible with SymPy. The compiler should give an error message about any conflicts like this, so I think it's not a big problem.

I think S won't be used in non-symbolic code, so it's not a problem. In symbolic code like inside SymPy I can imagine S as a type being used a lot, so I think it helps if it is short.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I reviewed by hand as much as I could. I think everything works!

@certik certik merged commit d6a6d8b into lcompilers:main Jun 19, 2023
@certik
Copy link
Contributor

certik commented Jun 19, 2023

Great job @anutosh491, this was the hardest. This provides the infrastructure and from here things should go a lot easier. Thank you @Shaikh-Ubaid for your review and help. Thanks @Thirumalai-Shaktivel for your great help with this.

@anutosh491 I would say from here, the next step is to add more symbolic features. Just add new tests and let's start supporting all the arithmetic operations, functions like sin/cos, operations like limit, integrate, diff, series, etc. Just use the C backend and the existing CI setup. As you start adding things, we'll have to iterate on the ASR design as needed. We'll have to eventually figure out how to free things correctly (basic_free in symengine). Keep the PRs small, just send many of them. We should be able to review & merge quickly from now on.

After we have a reasonable subset working, then we'll tackle the LLVM backend and get all the symbolic tests working with LLVM also.

@anutosh491
Copy link
Collaborator Author

Thanks a lot everyone who guided me on the PR . I'll send the second PR by today !

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.

5 participants