implemented parsing (string -> Basic) #772

srajangarg · 2016-01-20T17:52:45Z

Implement a basic integer parser, with all basic operations
Add support for symbols (any token which has even one non-digit character, is considered a symbol)
Change styling and formatting according to symengine, comment code properly
Add support for symengine implemented functions
Add support for symengine implemented constants
Handle most errors, and their reporting
Handle entered doubles (any numerical token with exactly a single '.' is converted to RealDouble)
Add support for function_symbol
Add support for complex numbers

Addresses #559

certik · 2016-01-20T20:32:48Z

I think this looks nice and clean. My only comment is about naming conventions --- opPrecedence -> op_precedence, operatorClose -> operator_close, etc.

abinashmeher999 · 2016-01-21T04:56:22Z

symengine/parser.cpp

+
+namespace SymEngine {
+
+class expressionParser


Class name should start with Caps.

abinashmeher999 · 2016-01-21T04:58:20Z

I think all source is in cpp currently. We should have one header file too at the end, if this is just for testing.

abinashmeher999 · 2016-01-21T05:07:04Z

symengine/parser.cpp

+
+public:
+
+    RCP<const Basic> parse_string(unsigned int l, unsigned int h)


Can you think of any case where it might be useful to expose this by making parse_string public? I didn't see this in the tests. If not, it would be better to keep it private.

srajangarg · 2016-01-21T12:51:39Z

Will get it done. Right now, my main focus is getting the function to work correctly, and including all features. Variable names, formatting and trivial changes, I'll make later.

isuruf · 2016-01-21T15:45:36Z

Looks good to me. If you have any doubts, please feel free to ask.
Another item for the TODO list

Add support for function_symbols

certik · 2016-01-21T15:47:18Z

symengine/parser.cpp

+        if(expr == "ln") return log(param1);
+
+        if(expr == "zeta") if(s[iter+1] != ',') return zeta(param1);
+        if(expr == "log") if(s[iter+1] != ',') return log(param1);


Instead of a linear search (i.e. a linear list of ifs), one can probably make it faster by checking std::map or something like that.

I really don't think anything faster than linear is possible. I have to search if 'any' of them matches, and do a unique thing on a match. I am not looking to 'find' an element in a array/map. Whatever way it is implemented, it will have to boil down to checking each the expr against each of these terms. (or something equivalent to these linear checks)

One complicated way to optimize maybe to make a search tree based on the letters of the expr. But I feel that is an overkill. Maybe do it after the main tasks?

Yes, we can see how to optimize this later.

Your current solution is O(n). A faster is to use O(log(n)) or O(1) using std::map or std::unordered_map, and you store a lambda function there that will do what you need.

srajangarg · 2016-01-21T21:46:12Z

@certik @isuruf Any ideas as to why build is failing on Windows, MinGW compiler?

certik · 2016-01-21T22:52:05Z

The mingw is failing because it can't find std::stoi in the std namespace. I assume some header file needs to be included?

srajangarg · 2016-01-22T07:55:38Z

http://www.cplusplus.com/reference/string/stoi/
Requires only <string>. It is a c++11 function though, "older compilers" might not support it.
What to do?

isuruf · 2016-01-23T08:33:09Z

std::stoi is not included in MinGW. It's a known bug.
You can use std::atoi(expr.c_str())

abhinavagarwal07 · 2016-01-23T17:02:04Z

some function arguments can be made const

isuruf · 2016-01-24T06:10:32Z

symengine/parser.cpp

+        {'*', 3}, {'/', 4}, {'^', 5}
+    };
+    std::map<std::string, RCP<const Basic> > constants = {
+        {"e", E}, {"EulerGamma", EulerGamma}, {"pi", pi}


Add I as a constant too. Then complex numbers will be supported as well.

Oh didn't think about that! Thanks!

isuruf · 2016-02-01T15:05:19Z

symengine/tests/basic/test_parser.cpp

+    // i am still not clear about the fundamentals of realdouble
+
+    s = "sqrt(2.0)+5";
+    // s = "sqrt(2)+5" this fails! (will the user add `.0` to integers to get them `computed`)


You can remove this line. It's supposed to work that way. If the user just wants numerics, then they are better off using a specialized library.

srajangarg · 2016-02-06T13:09:38Z

Any work to be done? @certik @isuruf

isuruf · 2016-02-06T13:12:18Z

symengine/parser.cpp

+            if (in[i] == ' ') {
+                continue;
+
+            } else if (in[i] == '*' and in[(i+1) % in.length()] == '*') {


% in.length() why is this there?

If in[i] is the last character of the entered string, in[i+1] goes out of bounds.

Then the best approach would be to check that i + 1 < in.length().

isuruf · 2016-02-07T17:11:08Z

Looks good to me. Can you add a header file parser.h and have just one method like RCP<const Basic> parse(const std::string& s) which would call the ExpressionParser in the implementation file?

Sumith1896 · 2016-02-08T11:51:20Z

Looks good to me too! +1

Sumith1896 · 2016-02-10T20:46:56Z

@certik Can you have a final look at this?

certik · 2016-02-10T21:02:56Z

symengine/parser.h

+
+#include <symengine/basic.h>
+#include <symengine/dict.h>
+#include <symengine/parser.cpp>


You should not include a .cpp file. Remove this include and just change

inline RCP<const Basic> parse(std::string& s) { ExpressionParser p; return p.parse_expr(s); }

to

RCP<const Basic> parse(const std::string& s);

and implement it in parser.cpp. Also make the input argument const, as I did.

certik · 2016-02-10T21:07:00Z

I left some comments. Otherwise it looks good to me as well.

srajangarg · 2016-02-10T22:21:46Z

@certik made the changes

certik · 2016-02-10T22:25:16Z

The header change is good, thanks. Please ping me once you fix the const thing, I'll do one final review.

srajangarg · 2016-02-10T22:39:16Z

done @certik

certik · 2016-02-10T22:45:37Z

+1 to merge as it is, if tests pass.

srajangarg · 2016-02-10T23:21:41Z

I'm not able to figure out what's wrong with the build. After changing all the maps to const, I'm getting a lot of error: passing ‘const '...’ as ‘this’ argument discards qualifiers [-fpermissive] on compilation.

certik · 2016-02-10T23:43:08Z

Ok, revert the const change, just to the header file change. In general, create git commits that are logical. So the header file change should go into its own commit. Since we both approved it, you just push it in and let it be. Then we can tackle the const thing, and if it's difficult, we can do it in a future PR.

certik · 2016-02-10T23:46:30Z

Btw, the reason it fails is because the operator[] for accessing std::map is not const, which I forgot about. See http://stackoverflow.com/questions/1474894/why-isnt-the-operator-const-for-stl-maps

Just forget about the const thing in this PR, fix the header and make sure all tests pass. Then we can merge this.

srajangarg · 2016-02-11T18:59:32Z

@certik done

certik · 2016-02-11T21:34:30Z

I think that this looks good. Thanks for the work!

implemented parsing (string -> Basic)

implemented parser for integers, basic operations, and tests

6d3b995

srajangarg force-pushed the parser branch from 1c23093 to 04844a5 Compare January 20, 2016 18:05

added support for symbols, and respective tests

04844a5

srajangarg force-pushed the parser branch from aa69ed3 to 679369d Compare January 20, 2016 20:30

changed include files, and started working on functions

679369d

srajangarg changed the title ~~implemented Parsing of string operations~~ [WIP] implemented parsing (string -> Basic) Jan 20, 2016

changed var names, added code for detecting commas for functions

b4a9ff5

abinashmeher999 reviewed Jan 21, 2016
View reviewed changes

symengine/parser.cpp

namespace SymEngine {

class expressionParser

Copy link

Contributor

abinashmeher999 Jan 21, 2016

Choose a reason for hiding this comment

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

Class name should start with Caps.

abinashmeher999 reviewed Jan 21, 2016
View reviewed changes

added full support for functions, and added tests (some doubts remain)

ef3a3cc

certik reviewed Jan 21, 2016
View reviewed changes

srajangarg mentioned this pull request Jan 21, 2016

[WIP] Some fixes and additions to trig functions #776

Merged

1 task

Merge branch 'master' into parser

3e32442

srajangarg force-pushed the parser branch from dc58415 to 4be43a1 Compare January 22, 2016 20:13

added support for symengine constants

4be43a1

added map from string to lambdas, to make faster search

9a504db

isuruf reviewed Jan 24, 2016
View reviewed changes

srajangarg force-pushed the parser branch from abd40dd to 9f6401d Compare January 24, 2016 07:10

added support for complex, by adding I to constants, also aaded consts

9f6401d

isuruf reviewed Feb 1, 2016
View reviewed changes

parser : minor fixes

b6c83a7

srajangarg changed the title ~~[WIP] implemented parsing (string -> Basic)~~ implemented parsing (string -> Basic) Feb 6, 2016

isuruf reviewed Feb 6, 2016
View reviewed changes

parser : minor fix

472cd35

srajangarg added 2 commits February 7, 2016 23:37

Merge branch 'master' into parser

184a679

added parser.h file, and fixed trailing whitespace in master

0c64af9

certik reviewed Feb 10, 2016
View reviewed changes

srajangarg force-pushed the parser branch from d50b439 to 5bb5817 Compare February 10, 2016 22:38

fixed function in parser.h

ad37dcd

srajangarg force-pushed the parser branch from 5bb5817 to ad37dcd Compare February 11, 2016 13:00

certik added a commit that referenced this pull request Feb 11, 2016

Merge pull request #772 from srajangarg/parser

450ecde

implemented parsing (string -> Basic)

certik merged commit 450ecde into symengine:master Feb 11, 2016

Sumith1896 mentioned this pull request Feb 24, 2016

Implement parsing of expressions #559

Closed

srajangarg deleted the parser branch June 6, 2016 05:42


		public:

		RCP<const Basic> parse_string(unsigned int l, unsigned int h)

implemented parsing (string -> Basic) #772

implemented parsing (string -> Basic) #772

Conversation

srajangarg commented Jan 20, 2016

certik commented Jan 20, 2016

Choose a reason for hiding this comment

abinashmeher999 commented Jan 21, 2016

Choose a reason for hiding this comment

srajangarg commented Jan 21, 2016

isuruf commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajangarg commented Jan 21, 2016

certik commented Jan 21, 2016

srajangarg commented Jan 22, 2016

isuruf commented Jan 23, 2016

abhinavagarwal07 commented Jan 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajangarg commented Feb 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isuruf commented Feb 7, 2016

Sumith1896 commented Feb 8, 2016

Sumith1896 commented Feb 10, 2016

Choose a reason for hiding this comment

certik commented Feb 10, 2016

srajangarg commented Feb 10, 2016

certik commented Feb 10, 2016

srajangarg commented Feb 10, 2016

certik commented Feb 10, 2016

srajangarg commented Feb 10, 2016

certik commented Feb 10, 2016

certik commented Feb 10, 2016

srajangarg commented Feb 11, 2016

certik commented Feb 11, 2016