Refactoring/API for parsing

[David Li]

Hello,

I have been working on the API for parse_expr after having implemented implicit multiplication, and would like your thoughts on my proposed changes. Currently in a local branch I have this:

• stringify_expr parses the input and returns a string (this will be useful for Gamma)
• eval_expr evaluates the input
• parse_expr combines the two for convenience/compatibility

The first two functions require a dictionary of globals; after seeing the discussion at PR 1648, would it be a good idea to add that option to parse_expr as well?

I have also changed the usage of implicit multiplication/other transformations to make it more convenient. Before the usage was

transformations = standard_transformations + (convert_xor, implicit_multiplication_application)

and now it is

transformations = (standard_transformations, convert_xor, implicit_multiplication_application)

as I have changed standard_transformations from being a tuple of functions to apply to a function. This doesn't save much typing, but it is cleaner.

sympify's API is not changed at all - as this is much more used, I don't want to break compatibility.

Some examples can be found in the tests of sympy_parser and implicit multiplication and in the docstring for parse_expr.

Additionally, I have fixed #2663 and #3423. Both were due to the automatic conversion of variables into symbols without taking into account context.

Are these changes acceptable? Also, does anyone have other ideas?

Thank you,

David

[Aaron Meurer]

On Nov 22, 2012, at 10:41 AM, David Li <li.da...@gmail.com> wrote:

Hello,

I have been working on the API for parse_expr after having implemented implicit multiplication, and would like your thoughts on my proposed changes. Currently in a local branch I have this:

• stringify_expr parses the input and returns a string (this will be useful for Gamma)
• eval_expr evaluates the input
• parse_expr combines the two for convenience/compatibility

Sounds good. 

The first two functions require a dictionary of globals; after seeing the discussion at PR 1648, would it be a good idea to add that option to parse_expr as well?

Yes. 

I have also changed the usage of implicit multiplication/other transformations to make it more convenient. Before the usage was

transformations = standard_transformations + (convert_xor, implicit_multiplication_application)

and now it is

transformations = (standard_transformations, convert_xor, implicit_multiplication_application)

as I have changed standard_transformations from being a tuple of functions to apply to a function. This doesn't save much typing, but it is cleaner.

What exactly was the tuple before?

sympify's API is not changed at all - as this is much more used, I don't want to break compatibility.

This is a good idea. 

Some examples can be found in the tests of sympy_parser and implicit multiplication and in the docstring for parse_expr.

Additionally, I have fixed #2663 and #3423. Both were due to the automatic conversion of variables into symbols without taking into account context.

Did you also fix comment 3 of 2663?

Can you make your fix generalized to an API?  I need this exact same fix to change isympy -a to use parsing.

Aaron Meurer

Are these changes acceptable? Also, does anyone have other ideas?

Thank you,

David

--
You received this message because you are subscribed to the Google Groups "sympy" group.
To post to this group, send email to sy...@googlegroups.com.
To unsubscribe from this group, send email to sympy+un...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/sympy?hl=en.

[David Li]

What exactly was the tuple before?

The tuple before was (auto_symbol, auto_number, factorial_notation). The new function that replaces the tuple iterates through those functions internally and applies each of them.

 

Did you also fix comment 3 of 2663?

I added those cases as tests, actually. 

Can you make your fix generalized to an API?  I need this exact same fix to change isympy -a to use parsing.

Can you clarify? So you want the fix refactored by itself? I'm not sure if that warrants a separate function, the change is simply (while iterating through the tokens)

if (# Don't convert attribute access

    (prevTok[0] == OP and prevTok[1] == '.') or

    # Don't convert keyword arguments

    (prevTok[0] == OP and prevTok[1] in ('(', ',')

     and nextTokNum == OP and nextTokVal == '=')):

    # Don't insert call to Symbol

else:

    # Insert call to Symbol

I'll put up a pull request soon.

David

[Aaron Meurer]

On Thu, Nov 22, 2012 at 6:28 PM, David Li <li.da...@gmail.com> wrote:

What exactly was the tuple before?

The tuple before was (auto_symbol, auto_number, factorial_notation). The new function that replaces the tuple iterates through those functions internally and applies each of them.

I like the API the way it currently is.  You are adding transformers to the standard transformers.  If you want, you can modify the standard transformers.  This cannot be done with your suggested change.

 

 

Did you also fix comment 3 of 2663?

I added those cases as tests, actually. 

Can you make your fix generalized to an API?  I need this exact same fix to change isympy -a to use parsing.

Can you clarify? So you want the fix refactored by itself? I'm not sure if that warrants a separate function, the change is simply (while iterating through the tokens)

if (# Don't convert attribute access

    (prevTok[0] == OP and prevTok[1] == '.') or

    # Don't convert keyword arguments

    (prevTok[0] == OP and prevTok[1] in ('(', ',')

     and nextTokNum == OP and nextTokVal == '=')):

Does that also include things like += ?

 

    # Don't insert call to Symbol

else:

    # Insert call to Symbol

Actually, I guess it won't be that simple, because I have to do it with ast, not tokenize.  See https://github.com/ipython/ipython/pull/2301 (and my work so far at https://github.com/asmeurer/sympy/tree/ast).  If you play with that branch, you'll see that isympy -a is broken, because it tries to transform stuff to Symbol() that it shouldn't.

But I guess there are just these three cases where a name cannot be wrapped in Symbol(): The name is the left-hand side of an assignment, the name is a attribute, or the name is a keyword argument (this is assuming that we already know from the parser that it is indeed a name and not a literal or a keyword).  We might want to examine the grammer specification to convince ourselves that these are indeed the only cases.

Aaron Meurer

  

I'll put up a pull request soon.

David

--

You received this message because you are subscribed to the Google Groups "sympy" group.

To view this discussion on the web visit https://groups.google.com/d/msg/sympy/-/HRg1zva3LesJ.

[David Li]

On Friday, November 23, 2012 2:41:36 AM UTC-7, Aaron Meurer wrote:

On Thu, Nov 22, 2012 at 6:28 PM, David Li <li.da...@gmail.com> wrote:

What exactly was the tuple before?

The tuple before was (auto_symbol, auto_number, factorial_notation). The new function that replaces the tuple iterates through those functions internally and applies each of them.

I like the API the way it currently is.  You are adding transformers to the standard transformers.  If you want, you can modify the standard transformers.  This cannot be done with your suggested change.

 

Alright, I'll revert that change.

 

 

Did you also fix comment 3 of 2663?

I added those cases as tests, actually. 

Can you make your fix generalized to an API?  I need this exact same fix to change isympy -a to use parsing.

Can you clarify? So you want the fix refactored by itself? I'm not sure if that warrants a separate function, the change is simply (while iterating through the tokens)

if (# Don't convert attribute access

    (prevTok[0] == OP and prevTok[1] == '.') or

    # Don't convert keyword arguments

    (prevTok[0] == OP and prevTok[1] in ('(', ',')

     and nextTokNum == OP and nextTokVal == '=')):

Does that also include things like += ?

I thought sympify didn't handle statements, only expressions?

David

[Aaron Meurer]

On Fri, Nov 23, 2012 at 9:53 AM, David Li <li.da...@gmail.com> wrote:

On Friday, November 23, 2012 2:41:36 AM UTC-7, Aaron Meurer wrote:

On Thu, Nov 22, 2012 at 6:28 PM, David Li <li.da...@gmail.com> wrote:

What exactly was the tuple before?

The tuple before was (auto_symbol, auto_number, factorial_notation). The new function that replaces the tuple iterates through those functions internally and applies each of them.

I like the API the way it currently is.  You are adding transformers to the standard transformers.  If you want, you can modify the standard transformers.  This cannot be done with your suggested change.

 

Alright, I'll revert that change.

 

 

Did you also fix comment 3 of 2663?

I added those cases as tests, actually. 

Can you make your fix generalized to an API?  I need this exact same fix to change isympy -a to use parsing.

Can you clarify? So you want the fix refactored by itself? I'm not sure if that warrants a separate function, the change is simply (while iterating through the tokens)

if (# Don't convert attribute access

    (prevTok[0] == OP and prevTok[1] == '.') or

    # Don't convert keyword arguments

    (prevTok[0] == OP and prevTok[1] in ('(', ',')

     and nextTokNum == OP and nextTokVal == '=')):

Does that also include things like += ?

I thought sympify didn't handle statements, only expressions?

Oh, OK.  So the = there is just for keyword argument then.

Aaron Meurer

 

David

 

 

    # Don't insert call to Symbol

else:

    # Insert call to Symbol

Actually, I guess it won't be that simple, because I have to do it with ast, not tokenize.  See https://github.com/ipython/ipython/pull/2301 (and my work so far at https://github.com/asmeurer/sympy/tree/ast).  If you play with that branch, you'll see that isympy -a is broken, because it tries to transform stuff to Symbol() that it shouldn't.

But I guess there are just these three cases where a name cannot be wrapped in Symbol(): The name is the left-hand side of an assignment, the name is a attribute, or the name is a keyword argument (this is assuming that we already know from the parser that it is indeed a name and not a literal or a keyword).  We might want to examine the grammer specification to convince ourselves that these are indeed the only cases.

Aaron Meurer

  

I'll put up a pull request soon.

David

--
You received this message because you are subscribed to the Google Groups "sympy" group.

To view this discussion on the web visit https://groups.google.com/d/msg/sympy/-/HRg1zva3LesJ.

To post to this group, send email to sy...@googlegroups.com.
To unsubscribe from this group, send email to sympy+un...@googlegroups.com.

For more options, visit this group at http://groups.google.com/group/sympy?hl=en.

--
You received this message because you are subscribed to the Google Groups "sympy" group.

To view this discussion on the web visit https://groups.google.com/d/msg/sympy/-/YJYyUzsxj20J.