Fix for Issue #1133 with Test by ShikharJ · Pull Request #1136 · symengine/symengine

ShikharJ · 2016-11-27T02:07:52Z

Please suggest any necessary changes if required.

ShikharJ · 2016-11-27T05:30:31Z

isuruf · 2016-11-27T15:50:17Z

Can you add some examples like exp(-x), exp(x) * 10, exp(-1) ?

certik · 2016-11-27T16:04:15Z

@ShikharJ I think that's the right fix. thanks for it. If you could add a few more tests as @isuruf suggested to ensure it works, that would be great.

ShikharJ · 2016-11-27T16:08:12Z

@certik @isuruf I'm on it.
Edit 1: Sympy produces this on input:

>>> exp(-1)
 -1
ℯ  
>>> exp(-x)
 -x
ℯ  
>>> 10*exp(x)
    x
10⋅ℯ 
>>> exp(x)
 x
ℯ

Edit 2: Using str printer:

>>> E**(-1)
exp(-1)
>>> E**(x)
exp(x)
>>> 10*E**(x)
10*exp(x)
>>> E**(-x)
exp(-x)

@isuruf
So I guess printing E**(-1) as exp(-1) rather than 1/exp(1) should do.

isuruf · 2016-11-28T04:20:17Z

@ShikharJ, with this, I think exp(-1) and exp(-1)*2 are printed differently.
Also with sympy, take a look at the string printer, not the pretty printer.

ShikharJ · 2016-11-28T07:53:29Z

@isuruf Indeed, exp(-1)*2 is being printed as 2/E. I'll try to fix that.

ShikharJ · 2016-12-01T18:01:59Z

@isuruf A review?

certik

Otherwise it looks good to me.

certik · 2016-12-01T19:05:58Z

symengine/printer.cpp

        s2 = s2.substr(0, s2.size() - 1);
        if (den > 1) {
            str_ = s + "/(" + s2 + ")";
+        } else if (s2[0] == 'e' and s2[1] == 'x' and s2[2] == 'p') {


Can this run out of bounds if len(s2) < 3?

Cannot this be written simply as s2.substr(3) == "exp"?

I'll make the changes.

ShikharJ · 2016-12-01T19:34:17Z

@certik Using s2.substr(3) == "exp" is setting off errors in other tests:

Test project /home/shikhar/symengine
      Start  1: test_rcp
 1/34 Test  #1: test_rcp .........................   Passed    0.01 sec
      Start  2: test_basic
 2/34 Test  #2: test_basic .......................   Passed    0.04 sec
      Start  3: test_arit
 3/34 Test  #3: test_arit ........................   Passed    0.13 sec
      Start  4: test_poly
 4/34 Test  #4: test_poly ........................   Passed    0.01 sec
      Start  5: test_series
 5/34 Test  #5: test_series ......................   Passed    0.01 sec
      Start  6: test_series_generic
 6/34 Test  #6: test_series_generic ..............   Passed    1.00 sec
      Start  7: test_functions
 7/34 Test  #7: test_functions ...................   Passed    0.10 sec
      Start  8: test_subs
 8/34 Test  #8: test_subs ........................   Passed    0.01 sec
      Start  9: test_integer
 9/34 Test  #9: test_integer .....................   Passed    0.01 sec
      Start 10: test_rational
10/34 Test #10: test_rational ....................   Passed    0.01 sec
      Start 11: test_number
11/34 Test #11: test_number ......................   Passed    0.01 sec
      Start 12: test_as_numer_denom
12/34 Test #12: test_as_numer_denom ..............   Passed    0.01 sec
      Start 13: test_parser
13/34 Test #13: test_parser ......................   Passed    0.02 sec
      Start 14: test_sets
14/34 Test #14: test_sets ........................   Passed    0.01 sec
      Start 15: test_fields
15/34 Test #15: test_fields ......................   Passed    0.14 sec
      Start 16: test_infinity
16/34 Test #16: test_infinity ....................   Passed    0.01 sec
      Start 17: test_ntheory
17/34 Test #17: test_ntheory .....................   Passed    0.08 sec
      Start 18: test_diophantine
18/34 Test #18: test_diophantine .................   Passed    0.03 sec
      Start 19: test_matrix
19/34 Test #19: test_matrix ......................   Passed    0.17 sec
      Start 20: test_printing
20/34 Test #20: test_printing ....................***Failed    0.01 sec
      Start 21: test_ccode
21/34 Test #21: test_ccode .......................   Passed    0.01 sec
      Start 22: test_uintpoly
22/34 Test #22: test_uintpoly ....................   Passed    0.01 sec
      Start 23: test_uratpoly
23/34 Test #23: test_uratpoly ....................   Passed    0.01 sec
      Start 24: test_mintpoly
24/34 Test #24: test_mintpoly ....................   Passed    0.01 sec
      Start 25: test_uexprpoly
25/34 Test #25: test_uexprpoly ...................   Passed    0.02 sec
      Start 26: test_mexprpoly
26/34 Test #26: test_mexprpoly ...................***Failed    0.04 sec
      Start 27: test_basic_conversions
27/34 Test #27: test_basic_conversions ...........   Passed    0.03 sec
      Start 28: test_evalf
28/34 Test #28: test_evalf .......................   Passed    0.01 sec
      Start 29: test_eval_double
29/34 Test #29: test_eval_double .................   Passed    0.01 sec
      Start 30: test_lambda_double
30/34 Test #30: test_lambda_double ...............   Passed    0.01 sec
      Start 31: test_cwrapper
31/34 Test #31: test_cwrapper ....................***Exception: Other  0.15 sec
      Start 32: test_expression
32/34 Test #32: test_expression ..................   Passed    0.03 sec
      Start 33: test_logic
33/34 Test #33: test_logic .......................   Passed    0.01 sec
      Start 34: test_finitediff
34/34 Test #34: test_finitediff ..................   Passed    0.02 sec

91% tests passed, 3 tests failed out of 34

Total Test time (real) =   2.21 sec

The following tests FAILED:
	 20 - test_printing (Failed)
	 26 - test_mexprpoly (Failed)
	 31 - test_cwrapper (OTHER_FAULT)
Errors while running CTest

certik · 2016-12-01T19:37:45Z

Post the output of ctest --output-on-failure, also make sure you are in Debug mode.

ShikharJ · 2016-12-01T19:44:11Z

@certik

Test project /home/shikhar/symengine
      Start  1: test_rcp
 1/34 Test  #1: test_rcp .........................   Passed    0.01 sec
      Start  2: test_basic
 2/34 Test  #2: test_basic .......................   Passed    0.04 sec
      Start  3: test_arit
 3/34 Test  #3: test_arit ........................   Passed    0.13 sec
      Start  4: test_poly
 4/34 Test  #4: test_poly ........................   Passed    0.02 sec
      Start  5: test_series
 5/34 Test  #5: test_series ......................   Passed    0.02 sec
      Start  6: test_series_generic
 6/34 Test  #6: test_series_generic ..............   Passed    1.00 sec
      Start  7: test_functions
 7/34 Test  #7: test_functions ...................   Passed    0.10 sec
      Start  8: test_subs
 8/34 Test  #8: test_subs ........................   Passed    0.01 sec
      Start  9: test_integer
 9/34 Test  #9: test_integer .....................   Passed    0.01 sec
      Start 10: test_rational
10/34 Test #10: test_rational ....................   Passed    0.01 sec
      Start 11: test_number
11/34 Test #11: test_number ......................   Passed    0.01 sec
      Start 12: test_as_numer_denom
12/34 Test #12: test_as_numer_denom ..............   Passed    0.01 sec
      Start 13: test_parser
13/34 Test #13: test_parser ......................   Passed    0.02 sec
      Start 14: test_sets
14/34 Test #14: test_sets ........................   Passed    0.01 sec
      Start 15: test_fields
15/34 Test #15: test_fields ......................   Passed    0.14 sec
      Start 16: test_infinity
16/34 Test #16: test_infinity ....................   Passed    0.01 sec
      Start 17: test_ntheory
17/34 Test #17: test_ntheory .....................   Passed    0.08 sec
      Start 18: test_diophantine
18/34 Test #18: test_diophantine .................   Passed    0.02 sec
      Start 19: test_matrix
19/34 Test #19: test_matrix ......................   Passed    0.13 sec
      Start 20: test_printing
20/34 Test #20: test_printing ....................***Failed    0.02 sec

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_printing is a Catch v1.1 b14 (develop) host application.
Run with -? for options

-------------------------------------------------------------------------------
test_printing(): printing
-------------------------------------------------------------------------------
/home/shikhar/symengine/symengine/tests/printing/test_printing.cpp:58
...............................................................................

/home/shikhar/symengine/symengine/tests/printing/test_printing.cpp:93: FAILED:
  REQUIRE( r->__str__() == "2*exp(-1)" )
with expansion:
  "2/exp(-1)" == "2*exp(-1)"

 _____           _____         _         
|   __|_ _ _____|   __|___ ___|_|___ ___ 
|__   | | |     |   __|   | . | |   | -_|
|_____|_  |_|_|_|_____|_|_|_  |_|_|_|___|
      |___|               |___|          

===============================================================================
test cases:  9 |  8 passed | 1 failed
assertions: 55 | 54 passed | 1 failed


      Start 21: test_ccode
21/34 Test #21: test_ccode .......................   Passed    0.01 sec
      Start 22: test_uintpoly
22/34 Test #22: test_uintpoly ....................   Passed    0.01 sec
      Start 23: test_uratpoly
23/34 Test #23: test_uratpoly ....................   Passed    0.01 sec
      Start 24: test_mintpoly
24/34 Test #24: test_mintpoly ....................   Passed    0.02 sec
      Start 25: test_uexprpoly
25/34 Test #25: test_uexprpoly ...................   Passed    0.02 sec
      Start 26: test_mexprpoly
26/34 Test #26: test_mexprpoly ...................***Failed    0.04 sec

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_mexprpoly is a Catch v1.1 b14 (develop) host application.
Run with -? for options

-------------------------------------------------------------------------------
Testing MExprPoly::eval
-------------------------------------------------------------------------------
/home/shikhar/symengine/symengine/tests/polynomial/test_mexprpoly.cpp:142
...............................................................................

/home/shikhar/symengine/symengine/tests/polynomial/test_mexprpoly.cpp:177: FAILED:
  REQUIRE( p->eval(m1) == expr4 )
due to unexpected exception with message:
  basic_string::substr: __pos (which is 3) > this->size() (which is 1)

===============================================================================
test cases: 16 | 15 passed | 1 failed
assertions: 88 | 87 passed | 1 failed


      Start 27: test_basic_conversions
27/34 Test #27: test_basic_conversions ...........   Passed    0.03 sec
      Start 28: test_evalf
28/34 Test #28: test_evalf .......................   Passed    0.01 sec
      Start 29: test_eval_double
29/34 Test #29: test_eval_double .................   Passed    0.01 sec
      Start 30: test_lambda_double
30/34 Test #30: test_lambda_double ...............   Passed    0.01 sec
      Start 31: test_cwrapper
31/34 Test #31: test_cwrapper ....................***Exception: Other  0.15 sec
terminate called after throwing an instance of 'std::out_of_range'
  what():  basic_string::substr: __pos (which is 3) > this->size() (which is 1)

      Start 32: test_expression
32/34 Test #32: test_expression ..................   Passed    0.02 sec
      Start 33: test_logic
33/34 Test #33: test_logic .......................   Passed    0.01 sec
      Start 34: test_finitediff
34/34 Test #34: test_finitediff ..................   Passed    0.02 sec

91% tests passed, 3 tests failed out of 34

Total Test time (real) =   2.19 sec

The following tests FAILED:
	 20 - test_printing (Failed)
	 26 - test_mexprpoly (Failed)
	 31 - test_cwrapper (OTHER_FAULT)
Errors while running CTest

certik · 2016-12-01T20:02:00Z

I see, the line should have been:

s2.substr(0, 3) == "exp"

See http://en.cppreference.com/w/cpp/string/basic_string/substr

isuruf · 2016-12-02T04:56:14Z

symengine/printer.cpp

        if (den > 1) {
            str_ = s + "/(" + s2 + ")";
+        } else if (s2.substr(0, 3) == "exp") {
+            str_ = s + "*" + s2;


This logic will fail when it's something like exp(-1)/x

isuruf · 2016-12-02T04:57:48Z

symengine/printer.cpp

            || (is_a<Rational>(*p.second)
                and rcp_static_cast<const Rational>(p.second)->is_negative())) {
-            if (eq(*(p.second), *minus_one)) {
+            if (eq(*(p.second), *minus_one) and neq(*(p.first), *E)) {


You need to use this check in the if statement in line no. 362. Then all the other changes below are not needed.

isuruf · 2016-12-02T08:03:48Z

symengine/printer.cpp


    for (const auto &p : x.dict_) {
-        if ((is_a<Integer>(*p.second)
+        if ((is_a<Integer>(*p.second) and neq(*(p.first), *E)


This should be for the whole || not just integer condition.

@isuruf Like this?

if (((is_a<Integer>(*p.second) and rcp_static_cast<const Integer>(p.second)->is_negative()) || (is_a<Rational>(*p.second) and rcp_static_cast<const Rational>(p.second)->is_negative())) and neq(*(p.first), *E))

Yes, you can simplify this a bit by doing,

if ((is_a<Integer>(*p.second) or is_a<Rational>(*p.second)) and static_cast<const Number &>(*p.second).is_negative() and neq(*p.first, *E))

isuruf · 2016-12-02T08:07:36Z

symengine/printer.cpp

-    if (eq(*b, *rational(1, 2))) {
+    if (eq(*b, *rational(1, 2)) and neq(*a, *E)) {
        o << "sqrt(" << apply(a) << ")";
+    } else if (eq(*a, *E)) {


Can you make this the first condition, so that it is simpler ?

Additional Fix using substr() Incorporated Suggestions Corrections Fix for Negative Exponents and Additional Changes

ShikharJ force-pushed the Issue1133 branch from aee2a02 to 6ee6bc9 Compare November 27, 2016 17:05

ShikharJ added 2 commits December 1, 2016 08:55

Added else if statement in _print_pow()

beb4f24

Added Further Tests

7bbdeff

ShikharJ force-pushed the Issue1133 branch from 6ee6bc9 to 40affaa Compare December 1, 2016 15:41

certik requested changes Dec 1, 2016

View reviewed changes

isuruf reviewed Dec 2, 2016

View reviewed changes

Fix for Negative Exponents and Additional Changes

30ccef2

Additional Fix using substr() Incorporated Suggestions Corrections Fix for Negative Exponents and Additional Changes

ShikharJ force-pushed the Issue1133 branch from 91c5df0 to 30ccef2 Compare December 2, 2016 09:08

isuruf approved these changes Dec 2, 2016

View reviewed changes

certik approved these changes Dec 2, 2016

View reviewed changes

isuruf merged commit cd3b64a into symengine:master Dec 3, 2016

ShikharJ deleted the Issue1133 branch December 3, 2016 04:43

Conversation

ShikharJ commented Nov 27, 2016 • edited by isuruf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShikharJ commented Nov 27, 2016

Uh oh!

isuruf commented Nov 27, 2016

Uh oh!

certik commented Nov 27, 2016

Uh oh!

ShikharJ commented Nov 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isuruf commented Nov 28, 2016

Uh oh!

ShikharJ commented Nov 28, 2016

Uh oh!

ShikharJ commented Dec 1, 2016

Uh oh!

certik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShikharJ commented Dec 1, 2016

Uh oh!

certik commented Dec 1, 2016

Uh oh!

ShikharJ commented Dec 1, 2016

Uh oh!

certik commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isuruf Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ShikharJ commented Nov 27, 2016 •

edited by isuruf

Loading

ShikharJ commented Nov 27, 2016 •

edited

Loading

isuruf Dec 2, 2016 •

edited

Loading