implement as_numer_denom #731

srajangarg · 2016-01-01T08:51:18Z

Working on adding as_numer_denom from SymPy

isuruf · 2016-01-01T10:53:12Z

@srajangarg take a look at one of the Visitor classes like EvalRealDoubleVisitor to see how to do this.

srajangarg · 2016-01-02T12:24:36Z

@isuruf Does the structure look ok now?

isuruf · 2016-01-02T13:39:10Z

symengine/basic.cpp

@@ -47,5 +48,11 @@ RCP<const Basic> Basic::diff(const RCP<const Symbol> &x) const
    return Derivative::create(rcp_from_this(), {x});
 }

+void Basic::as_numer_denom(const Ptr<RCP<const Basic>> &numer, const Ptr<RCP<const Basic>> &denom) const


You can move this to numer_denom.cpp

certik · 2016-01-03T05:38:25Z

symengine/tests/basic/test_as_numer_denom.cpp

+    REQUIRE(eq(*den, *one));
+
+    r1 = add(x, mul(y, pow(x, y)));
+    as_numer_denom(r1, outArg(num), outArg(den));


There should be a test what the result in num and den is here.

certik · 2016-01-03T05:40:02Z

@srajangarg I think this looks very good and clean. I left a small comment above.

isuruf · 2016-01-03T17:51:38Z

symengine/numer_denom.cpp

+namespace SymEngine {
+
+// need a .h file to fix this
+void as_numer_denom(const RCP<const Basic> &x, const Ptr<RCP<const Basic>> &numer, const Ptr<RCP<const Basic>> &denom);


You can move this declaration to basic.h

isuruf · 2016-01-03T17:52:37Z

One other class that you can be separated to numerator and denominator is Complex

srajangarg · 2016-01-06T15:25:38Z

@certik @isuruf
The PR adressing #681, seems to be complete from my side. Do you have any more ideas?

certik · 2016-01-06T17:01:44Z

symengine/basic.h

@@ -220,6 +220,7 @@ bool is_a_sub(const Basic &b);
 RCP<const Basic> expand(const RCP<const Basic> &self);
 umap_short_basic series(const RCP<const Basic> &ex, const RCP<const Symbol> &var, unsigned int prec);
 umap_short_basic series_invfunc(const RCP<const Basic> &ex, const RCP<const Symbol> &var, unsigned int prec);
+void as_numer_denom(const RCP<const Basic> &x, const Ptr<RCP<const Basic>> &numer, const Ptr<RCP<const Basic>> &denom);


Why not move this into numer_denom.h? This algorithm doesn't need anything from the core, so I think it's cleaner if it lives separate from the core, isn't it?

@isuruf suggested that I don't really need a .h file. I'm not the best judge as to what should be done, as I don't have the full vision of how the structure is to be built in the future. I'll stick to whatever you say.

Let's just keep it as it is. We can always move things around later.

certik · 2016-01-06T17:08:21Z

symengine/numer_denom.cpp

+            // if (eq(*divx_den, *one)) {
+            //     curr_num = add(mul(arg_num, divx), curr_num);
+            //     continue;
+            // } !! The below two 'general' statements cover this case


Should the commented code be removed?

I think we should let it stay. Just explains what I've done. Always good, if changes are to be made in the future.

I don't understand what the commented code is doing, or supposed to do. Is it a simplified version of the two lines below? It doesn't seem to assign to curr_den.

It's the case where the arg_den completely divides the curr_den (opposite of the previous case). Now as arg_den completely divides curr_den, curr_den doesn't have to be changed! After I check the two 'divide each other cases' I write the general version of 'multiplying denominators' (in an LCM sort of way) , by doing
curr_den = mul(curr_den, divx_den); and
curr_num = add(mul(curr_num, divx_den), mul(arg_num, divx_num));

We see that in the case commented out, divx_den was anyways going to be one and divx = divx_num, which would be anyways handled by the two lines (written above) and give the same result as expected.

curr_den = mul(curr_den, one); becomes curr_den = curr_den and
curr_num = add(mul(curr_num, one), mul(arg_num, divx)); becomes
curr_num = add(curr_num, mul(arg_num, divx));

Ah I see --- so the if statement is a special case, but it turns out that your general case already handles the special case. Is that right? Why don't you summarize what you just wrote in a small comment in the code to make this clear?

certik · 2016-01-06T17:09:15Z

@srajangarg I think that this looks very good, I only left two minor comments/questions. Thanks for the work.

certik · 2016-01-07T20:28:10Z

Thanks. I restarted the failing build on Travis.

certik · 2016-01-07T21:34:47Z

That looks good to me and tests pass. @isuruf any other comments?

Sumith1896 · 2016-01-07T21:53:16Z

+1 from my side too

implement as_numer_denom

certik · 2016-01-08T15:53:59Z

Thanks!

srajangarg changed the title ~~implement as_numer_denom method~~ [WIP] implement as_numer_denom Jan 1, 2016

srajangarg changed the title ~~[WIP] implement as_numer_denom~~ [ci skip] [WIP] implement as_numer_denom Jan 1, 2016

srajangarg changed the title ~~[ci skip] [WIP] implement as_numer_denom~~ [WIP] implement as_numer_denom Jan 1, 2016

srajangarg force-pushed the as_numer_denom branch from 5ee5442 to 69b6565 Compare January 1, 2016 10:09

srajangarg force-pushed the as_numer_denom branch 2 times, most recently from 19c3b31 to 151452e Compare January 2, 2016 11:33

added as_numer_denom function to Basic, using new visitor, added tests

151452e

isuruf reviewed Jan 2, 2016
View reviewed changes

srajangarg mentioned this pull request Jan 2, 2016

[WIP] DiffVisitor #734

Closed

3 tasks

removed as_numer_denom from Basic, also added support for mul

730e105

certik reviewed Jan 3, 2016
View reviewed changes

isuruf reviewed Jan 3, 2016
View reviewed changes

srajangarg force-pushed the as_numer_denom branch 4 times, most recently from 5e70dac to 589ad4d Compare January 3, 2016 18:54

srajangarg added 3 commits January 4, 2016 00:24

added funciton for Pow, and respective tests, and misc fixes

589ad4d

added AsNumerDenom for Add, added tests

abacf4d

Merge branch 'master' into as_numer_denom

fcbb980

srajangarg force-pushed the as_numer_denom branch from b8f731c to fcbb980 Compare January 5, 2016 07:48

fixed conflict w master

42f11f2

srajangarg force-pushed the as_numer_denom branch from ef0f7bc to 42f11f2 Compare January 5, 2016 17:44

added support for complex, and respective tests

d4949c2

certik reviewed Jan 6, 2016
View reviewed changes

certik changed the title ~~[WIP] implement as_numer_denom~~ implement as_numer_denom Jan 6, 2016

made small changes in the commented code in numer_denom.cpp

8dd1a81

certik added a commit that referenced this pull request Jan 8, 2016

Merge pull request #731 from srajangarg/as_numer_denom

62a324b

implement as_numer_denom

certik merged commit 62a324b into symengine:master Jan 8, 2016

srajangarg deleted the as_numer_denom branch January 9, 2016 12:57

isuruf mentioned this pull request Jan 21, 2016

Implement SymPy's as_numer_denom in SymEngine #681

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement as_numer_denom #731

implement as_numer_denom #731

srajangarg commented Jan 1, 2016

isuruf commented Jan 1, 2016

srajangarg commented Jan 2, 2016

isuruf Jan 2, 2016

certik Jan 3, 2016

certik commented Jan 3, 2016

isuruf Jan 3, 2016

isuruf commented Jan 3, 2016

srajangarg commented Jan 6, 2016

certik Jan 6, 2016

srajangarg Jan 6, 2016

certik Jan 7, 2016

certik Jan 6, 2016

srajangarg Jan 6, 2016

certik Jan 6, 2016

srajangarg Jan 7, 2016

certik Jan 7, 2016

srajangarg Jan 7, 2016

certik commented Jan 6, 2016

certik commented Jan 7, 2016

certik commented Jan 7, 2016

Sumith1896 commented Jan 7, 2016

certik commented Jan 8, 2016

implement as_numer_denom #731

implement as_numer_denom #731

Conversation

srajangarg commented Jan 1, 2016

isuruf commented Jan 1, 2016

srajangarg commented Jan 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certik commented Jan 3, 2016

Choose a reason for hiding this comment

isuruf commented Jan 3, 2016

srajangarg commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

certik commented Jan 6, 2016

certik commented Jan 7, 2016

certik commented Jan 7, 2016

Sumith1896 commented Jan 7, 2016

certik commented Jan 8, 2016