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

Implementing get argument through design 1 #2399

Merged
merged 5 commits into from
Oct 31, 2023

Conversation

anutosh491
Copy link
Collaborator

This pr is essentially trying to achieve the same thing as #2396 through a different design (Design 1 from #2393 which is #2393 (comment))

So essentially we can do something like the following

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

def main0():
    x: S = Symbol("x")
    y: S = Symbol("y")
    x = x**y
    arg1: S = x.args[1]
    print(arg1)

(lf) anutosh491@spbhat68:~/lpython/lpython$ lpython --enable-symengine examples/expr2.py 
y

@anutosh491
Copy link
Collaborator Author

anutosh491 commented Oct 28, 2023

There are 4 statements in total we need to consider as pointed out in the pr

    args: CPtr = vecbasic_new()
    basic_get_args(x, args)
    if vecbasic_size(args) <= i:
        raise
    vecbasic_get(args, i, ele)

Once we do these we should be good to go.

@certik
Copy link
Contributor

certik commented Oct 28, 2023

I think it's fine, let's get this PR to fully work. That gets something going. We can later iterate on it and do another design and compare if it is simpler, but the most important thing is to get something working, to unblock us. So this PR seems fine.

@anutosh491
Copy link
Collaborator Author

I have a doubt. I was adding a test for the same but this is what I discover. Consdier we have x + y and we use the args method on it.
Through sympy

>>> x + y
x + y
>>> _.args
(x, y)

Through python-symengine

>>> from symengine import var
>>> var("x y z")
(x, y, z)
>>> (x + y).args
(x, y)

But I think through symengine we would get y as the first element rather than x as the first argument

def main0():
    _x: i64 = i64(0)
    x: CPtr = empty_c_void_p()
    p_c_pointer(pointer(_x, i64), x)
    basic_new_stack(x)
    symbol_set(x, "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")
    _z: i64 = i64(0)
    z: CPtr = empty_c_void_p()
    p_c_pointer(pointer(_z, i64), z)
    basic_new_stack(z)
    basic_add(z, x, y)
    _ele1: i64 = i64(0)
    ele1: CPtr = empty_c_void_p()
    p_c_pointer(pointer(_ele1, i64), ele1)
    args: CPtr = vecbasic_new()
    basic_get_args(z, args)
    vecbasic_get(args, 0, ele1)
    print(basic_str(ele1))

This gives back y.
Hence after adding the tests, all symbolic tests pass through the llvm_sym and the c_sym backend but the cpython_sym backend fails because this check for compatibility with SymPy which fails for a simple addition operation.

Rest binary operators like mul, div,pow don't show the above behaviour.

@certik
Copy link
Contributor

certik commented Oct 30, 2023

Yes, this is another issue: the order of arguments for addition. This is determined by the hash table inside SymEngine. We could sort them lexicographically, but this will make it even more slower. Let's investigate Mathematica, if it keeps a stable order of arguments.

The way I would write the tests for now is to check that it returns (x, y) or (y, x).

TODO for this PR:

  • add a test that is agnostic of the order of (x, y)

@certik certik marked this pull request as draft October 30, 2023 16:01
@anutosh491 anutosh491 marked this pull request as ready for review October 31, 2023 10:37

// Define necessary variables
ASR::ttype_t* CPtr_type = ASRUtils::TYPE(ASR::make_CPtr_t(al, loc));
std::string args_str = current_scope->get_unique_name("_lcompilers_symbolic_argument_container");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went over the code again and everything looks fine to me. If you have any other name for our argument container other than what I've used, you could let me know.

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 think this looks good, thanks!

@certik certik merged commit da8d25b into lcompilers:main Oct 31, 2023
13 checks passed
@anutosh491 anutosh491 mentioned this pull request Jan 31, 2024
@anutosh491 anutosh491 deleted the Second_method_for_args branch May 28, 2024 06:03
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.

2 participants