Parser Improvements and Additions #1298

ShikharJ · 2017-06-21T17:05:41Z

No description provided.

ShikharJ · 2017-06-21T17:08:17Z

This currently gives an error as:

/home/shikhar/symengine/symengine/parser.cpp:116:47: error: could not convert ‘{{"Eq", SymEngine::Eq}, {"Ne", SymEngine::Ne}, {"Ge", SymEngine::Ge}, {"Gt", SymEngine::Gt}, {"Le", SymEngine::Le}, {"Lt", SymEngine::Lt}}’ from ‘<brace-enclosed initializer list>’ to ‘std::map<std::__cxx11::basic_string<char>, std::function<Teuchos::RCP<const SymEngine::Boolean>(const Teuchos::RCP<const SymEngine::Basic>&, const Teuchos::RCP<const SymEngine::Basic>&)> >’
             {"Gt", Gt}, {"Le", Le}, {"Lt", Lt}};

isuruf · 2017-06-21T17:09:30Z

Looks good to me. It'll also be useful to be able to parse strings like x < y as well.

ShikharJ · 2017-06-21T20:21:01Z

@isuruf Can you review? I'm not sure this is how it is supposed to be implemented.

srajangarg · 2017-06-22T06:16:59Z

symengine/parser.cpp

+    std::map<std::string,
+             std::function<RCP<const Boolean>(const RCP<const Basic> &,
+                                              const RCP<const Basic> &)>>
+        double_arg_boolean_functions = {


Can you rename this to boolean_functions?
Edit : nevermind

The compilation error is possibly due to the overloaded Eq function. See how I have done it for the overloaded log function above. You have to cast it to a specific function type (in this case the double argument variant)

srajangarg · 2017-06-22T06:18:40Z

symengine/parser.cpp

@@ -119,7 +131,7 @@ class ExpressionParser
    // the string to be parsed, obtained after removing all spaces from input
    // string
    std::string s;
-    // it's length
+    // its length


Grammar nazi! 😛

srajangarg · 2017-06-22T07:18:41Z

symengine/parser.cpp

+                                parse_string(iter + 1, operator_end[iter]));
+                    iter = operator_end[iter] - 1;
+
+                } else if (s[iter] == '<' and s[iter + 1] == '=') {


can iter be the last index? s[iter+1] may segfault

I have my doubts about that as well. A hacky alternative that occurred to me was to use other symbols, such as # or @ for replacing all the instances of <= and >=, just like it's being done for ** to ^ during preprocessing. What would you suggest?

No that isn't a good solution. For now, will checking iter + 1 < end work? (or <= I don't remember exactly)

It doesn't. Each time the terminal returns Operator Inconsistency!.

Did you try and look into this further? Why does adding && iter + 1 <= end cause it to throw everytime?

Yes. Tried for < as well as <=.

@srajangarg Though I'm not sure, probably the error occurs in parse_expr() where x >= y get split into x > and the rest. I think it is during the simplification of this expression that the error is thrown up.

So, ideally to fix this we should move from a character based approach to a "token" based approach, where each token is one or more characters. Everything proceeds the same way but instead of iterating over characters we iterate over tokens. Tokens are generated in the parse_expr stage. You can think of ** being converted to ^ a tokenization itself (right now our tokens are only single characters, and we tokenize the multiple characters to a single one).

Do you think you can implement this? As we add more and more operators, using only single characters as tokens will become a problem (and we can soon run out of symbols). If you don't want to tackle this now, go ahead with the special symbol hacky approach.

I had tried changing the set<char> OPERATORS to set<std::string> OPERATORS and subsequently std::map<char, int> op_precedence to std::map<std::string, int> op_precedence. But these changes would require an overhaul of the current iterative algorithm. I'd like to open an issue, for now, and tackle it later.

srajangarg · 2017-06-22T07:20:15Z

I would like to see some more complicated test cases, and cases which will not be parsed correctly (ie will throw) using these new symbols

srajangarg · 2017-06-27T19:28:34Z

what does parsing sin(x < y) generate?

ShikharJ · 2017-06-27T19:37:00Z

That gives out an Operator Inconsistency error as well.
Edit: This turns out to give an error in SymPy as well (I've cut out most of the traceback):

In [1]: sin(x < y)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-e00934bdc3bd> in <module>()
----> 1 sin(x < y)
TypeError: cannot determine truth value of Relational

srajangarg · 2017-06-27T19:49:11Z

Should this be an error while parsing (ie. you cant have this string), or error while actually constructing the symbolic tree (ie. a relational inside a sin should not be allowed)?

I feel it should parse correctly. I'm not sure.

ShikharJ · 2017-06-27T19:53:02Z

I think it should be handled in the respective classes. More recently, like it is handled in Floor and Ceiling. SymPy handles this in the classes as well.

srajangarg · 2017-06-27T20:34:48Z

So then our parser throwing Operator Inconsistency is wrong in the case of sin(x < y) right? Figure out what's going wrong, and try to fix it.

ShikharJ · 2017-06-28T11:06:47Z

@srajangarg Can you review? The build failure is unrelated to the changes, probably that needs to be restarted. sin(x < y) currently returns sin(Lt(x, y)), though that can be fixed through another PR.

srajangarg · 2017-06-28T11:10:38Z

Is the operator precedence set properly? How is x + y < 2 parsed? Please add more extensive test cases, dealing with brackets, operators, functions etc.

ShikharJ · 2017-06-28T16:31:18Z

@srajangarg Can you restart the failing build? Also, should I add more tests?

isuruf · 2017-06-28T16:35:52Z

symengine/tests/basic/test_parser.cpp

+    res = parse(s);
+    CHECK(eq(*res, *Le(mul(x, y), add(x, y))));
+
+    s = "x - y = x/y";


= should not be used for equality. Use ==

isuruf · 2017-06-28T16:38:42Z

symengine/tests/basic/test_parser.cpp

+    res = parse(s);
+    CHECK(eq(*res, *Le(sub(x, y), div(x, y))));
+
+    s = "x = y < 2";


This test and all the tests below should be removed. They don't make sense.

These were implemented to check for operator precedence. I'll remove them.

ShikharJ · 2017-06-29T10:59:13Z

Ping @isuruf @srajangarg

srajangarg · 2017-06-29T11:01:30Z

LGTM

isuruf

One minor issue. Also can you try and see if And(x < y, w >= z) works ?

isuruf · 2017-06-29T14:59:33Z

symengine/tests/basic/test_parser.cpp

@@ -269,6 +362,7 @@ TEST_CASE("Parsing: constants", "[parser]")

    s = "E*pi";
    res = parse(s);
+    s = "2*(x+1)**10 + 3*(x+2)**5";


Why this change?

Sorry, this is accidental.

ShikharJ · 2017-06-29T18:15:07Z

@isuruf @srajangarg I've added support for some additional Boolean functions. Please review the last commit.

isuruf · 2017-07-01T04:18:18Z

symengine/parser.cpp

@@ -374,11 +485,27 @@ class ExpressionParser
        s.clear();
        s.reserve(in.length());

-        // Replacing ** with ^
+        // TODO: Implement multi-character operator parsing support


I don't really like this hack. Would it take a long time to implement this?

ShikharJ · 2017-07-06T21:14:30Z

symengine/parser.cpp

        for (unsigned int i = 0; i < in.length(); ++i) {
            if (in[i] == '*' and i + 1 < in.length() and in[i + 1] == '*') {
+                // Replacing ** with ^
                s += '^';


@isuruf @srajangarg Should this be removed? Is there a need to parse &, | or ^? Also, can you please review the PR?

Yes it should. If you've implemented multi character operator support.

srajangarg · 2017-07-10T10:35:39Z

First look this looks good. Give me some time to go through it fully.

But then again, this is just still a hacky solution. We need to switch to a proper lexer/parser based approach for this to be scalable in the long run.

ShikharJ · 2017-07-17T00:49:32Z

@isuruf What would be your take on this? I don't have a clear idea on implementing "tokenization" of operators, and as such, I'd like to open an issue for that instead.

Parser Improvements and Additions

isuruf requested a review from srajangarg June 22, 2017 02:09

srajangarg reviewed Jun 22, 2017

View reviewed changes

ShikharJ force-pushed the Parser branch from 5b02f76 to 73f0638 Compare June 22, 2017 09:25

ShikharJ force-pushed the Parser branch from 73f0638 to 6872abb Compare June 27, 2017 21:07

ShikharJ mentioned this pull request Jun 28, 2017

Relationals passed to functions should throw #1303

Closed

ShikharJ added 3 commits June 28, 2017 16:48

Grammatical Fixes

4a3355b

Add Relationals and NaN to Parser

91868e1

Add support for operators

2178e2a

ShikharJ force-pushed the Parser branch from 6872abb to e681374 Compare June 28, 2017 13:31

isuruf reviewed Jun 28, 2017

View reviewed changes

ShikharJ force-pushed the Parser branch from e681374 to 1f95560 Compare June 28, 2017 19:09

srajangarg requested a review from isuruf June 29, 2017 11:01

isuruf reviewed Jun 29, 2017

View reviewed changes

Make-shift fix for Le and Ge

97d2841

ShikharJ force-pushed the Parser branch from 1f95560 to 97d2841 Compare June 29, 2017 15:33

Add support for additional Boolean functions

64f1622

isuruf reviewed Jul 1, 2017

View reviewed changes

ShikharJ commented Jul 6, 2017

View reviewed changes

ShikharJ force-pushed the Parser branch 4 times, most recently from 2c4d09b to 700c5a0 Compare July 15, 2017 03:52

Implement multi-character operator support

4509ca1

ShikharJ force-pushed the Parser branch from 700c5a0 to 4509ca1 Compare July 18, 2017 14:13

srajangarg merged commit 2695a9d into symengine:master Jul 19, 2017

ShikharJ deleted the Parser branch July 19, 2017 09:20

ShikharJ mentioned this pull request Oct 28, 2017

Parse "^" causes segmentation fault? #1351

Closed

isuruf pushed a commit to isuruf/symengine that referenced this pull request Aug 4, 2018

Merge pull request symengine#1298 from ShikharJ/Parser

c7cd6bc

Parser Improvements and Additions

Parser Improvements and Additions #1298

Parser Improvements and Additions #1298

Conversation

ShikharJ commented Jun 21, 2017

ShikharJ commented Jun 21, 2017

isuruf commented Jun 21, 2017

ShikharJ commented Jun 21, 2017

srajangarg Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

srajangarg Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajangarg Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajangarg Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajangarg Jun 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajangarg Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajangarg commented Jun 22, 2017 • edited Loading

srajangarg commented Jun 27, 2017 • edited Loading

ShikharJ commented Jun 27, 2017 • edited Loading

srajangarg commented Jun 27, 2017

ShikharJ commented Jun 27, 2017

srajangarg commented Jun 27, 2017

ShikharJ commented Jun 28, 2017

srajangarg commented Jun 28, 2017

ShikharJ commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShikharJ commented Jun 29, 2017

srajangarg commented Jun 29, 2017

isuruf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShikharJ commented Jun 29, 2017

Choose a reason for hiding this comment

ShikharJ Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

srajangarg Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

srajangarg commented Jul 10, 2017 • edited Loading

ShikharJ commented Jul 17, 2017

srajangarg Jun 22, 2017 •

edited

Loading

srajangarg Jun 22, 2017 •

edited

Loading

srajangarg Jun 22, 2017 •

edited

Loading

srajangarg Jun 22, 2017 •

edited

Loading

srajangarg Jun 25, 2017 •

edited

Loading

srajangarg Jun 27, 2017 •

edited

Loading

srajangarg commented Jun 22, 2017 •

edited

Loading

srajangarg commented Jun 27, 2017 •

edited

Loading

ShikharJ commented Jun 27, 2017 •

edited

Loading

ShikharJ Jul 6, 2017 •

edited

Loading

srajangarg Jul 10, 2017 •

edited

Loading

srajangarg commented Jul 10, 2017 •

edited

Loading