Implements partial substitution of Mul objects by eeshan9815 · Pull Request #1395 · symengine/symengine

eeshan9815 · 2018-02-07T09:11:46Z

Fixes #1359
Added 11 test cases which demonstrate the partial substitution of Mul in mixed-variable terms as well.
ping @isuruf

isuruf-bot · 2018-02-07T09:12:16Z

Hi,

I've run clang-format and found that the code needs formatting.
Here's a commit that fixes this. isuruf-bot@47616f0

To use the commit you can do

curl -o format.diff https://github.com/isuruf-bot/symengine/commit/47616f0b062d66db394f188d633a3fa70444a07c.diff
git apply format.diff

ShikharJ · 2018-02-07T11:03:03Z

@eeshan9815 You can run ./bin/test_format_local.sh before committing the changes. That would automatically format the code.

eeshan9815 · 2018-02-07T15:25:41Z

@ShikharJ Okay, thanks!

eeshan9815 · 2018-02-08T15:48:20Z

@isuruf can you please review the PR?

isuruf · 2018-02-08T15:53:27Z

+                    }
+                } else
+                    d = x.get_dict();
+            } else if (is_a<Pow>(*sub1)) {


This is handled in the first part. Why check again?

@isuruf
The first part is the earlier implementation that only looks for exact matches, and not partial matches which should be handled using subs.
That part is retained only for the fast execution speeds in case of exact matches as it doesn't involve many heavy computations.

isuruf · 2018-02-08T16:07:56Z

Here's some test cases to handle,

In [14]: t = 2*x* y * z * w

In [15]: t.subs({x*y: 2,z*w:4})
Out[15]: 16

In [16]: t.subs({x*y: 2,z:1})
Out[16]: 4*w

In [17]: t.subs({2*x*y: 5,2*z*w:3})
Out[17]: 3*x*y

In [18]: t.subs({2*x*y: 5,z*w:3})
Out[18]: 15

eeshan9815 · 2018-02-08T20:28:03Z

In [17]: t.subs({2*x*y: 5,2*z*w:3})
Out[17]: 3*x*y

How should it be decided whether 3*x*y is returned or 5*z*w?

isuruf · 2018-02-08T20:29:10Z

How should it be decided whether 3xy is returned or 5zw?

Doesn't matter, use the first match.

eeshan9815 · 2018-02-10T00:18:57Z

Here's some test cases to handle,
In [14]: t = 2*x* y * z * w

In [15]: t.subs({x*y: 2,z*w:4})
Out[15]: 16

In [16]: t.subs({x*y: 2,z:1})
Out[16]: 4*w

In [17]: t.subs({2*x*y: 5,2*z*w:3})
Out[17]: 5*z*w

In [18]: t.subs({2*x*y: 5,z*w:3})
Out[18]: 15

I have added the above test cases to test_subs.cpp and all test cases are passing.
([17] matches to 5\*z\*w, not 3\*x\*y)

eeshan9815 · 2018-02-10T00:45:19Z

ping @isuruf

isuruf · 2018-02-10T00:48:36Z

+                    }
+                } else
+                    d = x.get_dict();
+            } else if (is_a<Symbol>(*sub1)) {


Can you explain this part? Why is sub1 being a Symbol important? I thought the issue was that sub1 being a Mul isn't handled correctly.

Partial substitution can happen for Symbol objects as well. For example, if the expression is x**2*y and sub1 is x, there is still partial substitution possible.
The first part only looks for exact matches.

Follow up question, should substitution happen repeatedly until not possible? For instance, if the expression is x**2*y**3 and x*y is to be replaced by z, should the result be x*y**2*z or y*z**2?

Hmm, (x**2*y).subs({x: z}) didn't work before?

It did, but I've made the non fast_exec implementation complete. Should I remove these redundant parts?

ping @isuruf

If it's redundant, remove them

@eeshan9815, sorry about the delay. Can you remove redundant code here?

certik · 2018-03-06T19:12:11Z

Does this PR affect performance?

eeshan9815 · 2018-03-07T09:14:11Z

@certik It is slower than the earlier implementation because that only naively looked at the expression for exact matches.
I have optimised the partial substitution part of the function by comparing and checking if the exponent of the expression to be substituted is smaller than that of the original expression rather than checking for exact matches of all the possibilities.
Also, if an exact match is found, it will be run as fast as the earlier implementation.

eeshan9815 · 2018-03-20T13:49:12Z

@isuruf is there more work to be done before merging?

eeshan9815 · 2018-05-23T14:01:29Z

ping @isuruf

Implements partial substitution of Mul objects

d47f40b

eeshan9815 force-pushed the subs-mul branch from a58a092 to d47f40b Compare February 7, 2018 09:16

isuruf reviewed Feb 8, 2018

View reviewed changes

Bug fix and added 4 test cases

693c5d6

isuruf reviewed Feb 10, 2018

View reviewed changes

Conversation

eeshan9815 commented Feb 7, 2018

Uh oh!

isuruf-bot commented Feb 7, 2018

Uh oh!

ShikharJ commented Feb 7, 2018

Uh oh!

eeshan9815 commented Feb 7, 2018

Uh oh!

eeshan9815 commented Feb 8, 2018

Uh oh!

isuruf Feb 8, 2018 • 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!

eeshan9815 Feb 9, 2018 • 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!

isuruf commented Feb 8, 2018

Uh oh!

eeshan9815 commented Feb 8, 2018

Uh oh!

isuruf commented Feb 8, 2018

Uh oh!

eeshan9815 commented Feb 10, 2018

Uh oh!

eeshan9815 commented Feb 10, 2018

Uh oh!

isuruf Feb 10, 2018

Choose a reason for hiding this comment

Uh oh!

eeshan9815 Feb 10, 2018

Choose a reason for hiding this comment

Uh oh!

isuruf Feb 10, 2018

Choose a reason for hiding this comment

Uh oh!

eeshan9815 Feb 10, 2018

Choose a reason for hiding this comment

Uh oh!

eeshan9815 Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

isuruf Feb 13, 2018

Choose a reason for hiding this comment

Uh oh!

isuruf Jun 10, 2018

Choose a reason for hiding this comment

Uh oh!

certik commented Mar 6, 2018

Uh oh!

eeshan9815 commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eeshan9815 commented Mar 20, 2018

Uh oh!

eeshan9815 commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

isuruf Feb 8, 2018 •

edited

Loading

eeshan9815 Feb 9, 2018 •

edited

Loading

eeshan9815 commented Mar 7, 2018 •

edited

Loading