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.

Sign up for GitHub

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

Ruby wrapper for parser #60

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Ruby wrapper for parser #60

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
588fa76
Ruby wrapper for parser
rajithv Jul 8, 2016
cf77487
Update symengine version
rajithv Jul 8, 2016
300c769
testing parse method
rajithv Jul 8, 2016
5980a4a
Merge branch 'master' of https://github.com/symengine/symengine.rb in…
rajithv Jul 8, 2016
a2f88e2
Merge branch 'master' of https://github.com/symengine/symengine.rb in…
rajithv Aug 21, 2016
c20e7a0
Exception Handling and test with parse error
rajithv Aug 21, 2016
7e9ca53
Added more tests for parser
rajithv Aug 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions ext/symengine/ruby_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,24 @@ VALUE cutils_sympify(VALUE self, VALUE operand)
sympify(operand, cbasic_operand);
result = Data_Wrap_Struct(Klass_of_Basic(cbasic_operand), NULL,
cbasic_free_heap, cbasic_operand);
return result;
}

VALUE cutils_parse(VALUE self, VALUE str)
{
VALUE result = Qnil;
basic_struct *cbasic_result = basic_new_heap();

char *cstr = RSTRING_PTR(str);

symengine_exceptions_t error_code = basic_parse(cbasic_result, cstr);

if (error_code == SYMENGINE_NO_EXCEPTION) {
result = Data_Wrap_Struct(Klass_of_Basic(cbasic_result), NULL,
cbasic_free_heap, cbasic_result);
} else {
raise_exception(error_code);
}
return result;
}

Expand All @@ -23,6 +40,7 @@ VALUE cutils_evalf(VALUE self, VALUE operand, VALUE prec, VALUE real)
cresult = basic_new_heap();

sympify(operand, cresult);

symengine_exceptions_t error_code
= basic_evalf(cresult, cresult, NUM2INT(prec), (real == Qtrue ? 1 : 0));

Expand Down
4 changes: 4 additions & 0 deletions ext/symengine/ruby_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,9 @@

// Returns the Ruby Value after going through sympify
VALUE cutils_sympify(VALUE self, VALUE operand);
// Parses the expression in str and returns a SymEngine::Basic
VALUE cutils_parse(VALUE self, VALUE str);
// Evaluates the numerical value in operand (SymEngine::Basic) to precision
// defined in prec
VALUE cutils_evalf(VALUE self, VALUE operand, VALUE prec, VALUE real);
#endif // RUBY_UTILS_H_
3 changes: 3 additions & 0 deletions ext/symengine/symengine.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ void Init_symengine()
rb_define_module_function(m_symengine, "convert", cutils_sympify, 1);
rb_define_global_function("SymEngine", cutils_sympify, 1);

// Parser as a Module Level Function
rb_define_module_function(m_symengine, "parse", cutils_parse, 1);

// evalf as a Module Level Function
rb_define_module_function(m_symengine, "_evalf", cutils_evalf, 3);

Expand Down
11 changes: 11 additions & 0 deletions spec/symengine_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,16 @@
it { is_expected.to be_a SymEngine::Rational }
its(:to_s) { is_expected.to eq '1/3' }
end

describe 'parse' do
subject { SymEngine::parse('123 + 321') }
Copy link
Member

Choose a reason for hiding this comment

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

Since these specs act as documentation, can you add more strings for parsing?
eg.

  • x + (y * 3 - 5)
  • x ** y + f(x)
  • sin(x) + cos(y) / 2
  • 1 / 2 + 3 / 2
  • 1 + 2 * 2
  • tan(0) - sqrt(2)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to add a few more to this list

  • 5+-3
  • 5--3

All Without spaces
And

  • 5++3 or something like this to raise an error.


it { is_expected.to be_a SymEngine::Integer }
it { is_expected.to eq 444 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

What will be if there's erroneous/unparseable input? (And, BTW, what input, if any is considered erroneous or unparseable by symengine?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zverok I did think about that. In C++ code it will throw errors for unparseable code. Checking that may almost be equivalent to doing the parsing. There are 4 errors,

  1. Expected token!
  2. Invalid symbol or number!
  3. Mismatching parantheses!
  4. Operator inconsistency!

Catching the exceptions from the C++ code and throwing ruby exceptions will be covered in the weeks 9 and 10. So this issue will be solved then.

As of now, I wanted the tests to merely confirm that the Ruby wrapper is communicating successfully with the cwrapper. Actually the same idea was behind the matrix wrapper tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. I just always became suspicious when I see tests for success and not tests for fail :)
In fact, I always prefer to add empty contexts for such a deferred tests pathes, like this:

context "When there is parsing error" do
  # TODO: will be covered on weeks 9-10
end

Though, I'm not insisting, currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a quite nice way to make it clear!

I'll keep this as it is, because week 9 is just one week away from now, probably I will start working on it even earlier.

end

it 'gives parse errors' do
expect { SymEngine::parse('12a + n34a9') }.to raise_error(RuntimeError)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the error message should be checked too since we don't have specific exception for ParseError.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to add ParseError as a specific exception? That seems worthwhile.

Copy link
Contributor

Choose a reason for hiding this comment

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

Found just the thing that explains creating custom exceptions from Ruby C API: http://clalance.blogspot.in/2011/01/writing-ruby-extensions-in-c-part-5.html

Copy link
Member

Choose a reason for hiding this comment

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

You don't have to create new exceptions.

  • SYMENGINE_RUNTIME_ERROR - rb_eRuntimeError
  • SYMENGINE_DIV_BY_ZERO - rb_eZeroDivError
  • SYMENGINE_NOT_IMPLEMENTED - rb_eNotImpError
  • SYMENGINE_PARSE_ERROR - rb_eSyntaxError
  • SYMENGINE_UNDEFINED - rb_eFloatDomainError ?? (I'm not sure what SYMENGINE_UNDEFINED is. Is this supposed to be a Domain error?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I partially agree to the above. While RuntimeError and ZeroDivisionError should be used, our code atleast shouldn't raise NotImplementedError and SyntaxError. The latter ones are ScriptErrors and not StandardErrors.
This post explains why it would be abuse of NotImplementedError. SyntaxError is not a right fit since it is raised while encountering invalid Ruby code. If the user wants to handle a parse error, (s)he will have to handle SyntaxError and that would also lead to unwanted silencing of the actual syntax error in the source code.

Also there is the technical problem that a rescue clause by default only catches the StandardErrors. It is highly advised that custom application level exceptions be derived from StandardError.

For SYMENGINE_UNDEFINED it can be just StandardError.

Copy link
Member

@isuruf isuruf Aug 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I just checked SciRuby/nmatrix repo and they use NotImplementedError and IOError (instead of SyntaxError)

end

end