Update solvers to handle binomial. #18802

sbt4104 · 2020-03-08T12:24:51Z

References to other Issues or PRs

Fixes #10993

Brief description of what is fixed or changed

Added conditions to expand binomial in solvers
Update expand function for binomial

Other comments

Please suggest changes.

Release Notes

solvers
- Fix solvers to handle binomials
functions
- Update expand function for binomials.

sympy-bot · 2020-03-08T12:25:01Z

✅

Hi, I am the SymPy bot (v158). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

functions
- Update expand function for binomials. (#18802 by @sbt4104)
solvers
- Fix solvers to handle binomials (#18802 by @sbt4104)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

#### References to other Issues or PRs
Fixes #10993

#### Brief description of what is fixed or changed
Added conditions to expand binomial in solvers
Update expand function for binomial
#### Other comments 
Please suggest changes.

### Release Notes
<!-- BEGIN RELEASE NOTES -->
* solvers
    - Fix solvers to handle binomials
* functions
    - Update expand function for binomials.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

oscarbenjamin · 2020-03-08T13:09:50Z

I'm not sure where this belongs but certainly not as the opening lines in the solve function. Unfortunately solve is a bit of a mess so there isn't necessarily a correct place to put something like this.

sbt4104 · 2020-03-08T13:42:25Z

I think putting those conditions in "preprocess equations" is better.

codecov · 2020-03-08T13:45:10Z

Codecov Report

Merging #18802 into master will increase coverage by 0.024%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##            master    #18802       +/-   ##
=============================================
+ Coverage   75.651%   75.676%   +0.024%     
=============================================
  Files          647       647               
  Lines       168420    168512       +92     
  Branches     39682     39705       +23     
=============================================
+ Hits        127413    127524      +111     
+ Misses       35454     35440       -14     
+ Partials      5553      5548        -5

sbt4104 · 2020-03-09T03:59:56Z

Please review and suggest the changes.

oscarbenjamin · 2020-03-09T09:56:21Z

Consider that you might have an expression involving a binomial like

In [4]: expr = 1 + binomial(x, 3)                                                                                                                             

In [5]: isinstance(expr, binomial)                                                                                                                            
Out[5]: False

In [6]: expr.has(binomial)                                                                                                                                    
Out[6]: True

We probably don't want to expand all functions so it would be better to use expr.replace to pick out the binomials and expand them.

This code is still not in the right place: solve hasn't even converted Eqs to Expr at that point. Look through solve to find where similar transformations are happening.

sbt4104 · 2020-03-09T15:00:43Z

I have made the changes so that only binomial terms are expanded and added the code after Eqs are checked and converted. Please check if it is correctly done.

sbt4104 · 2020-03-09T15:45:10Z

can someone explain to me why my test on travis-ci is failing.
$ bin/test_travis.sh
+[[ '' == \t\r\u\e ]]
+[[ '' == \t\r\u\e ]]
+[[ '' == \t\r\u\e ]]
+[[ '' == \t\r

sbt4104 · 2020-03-09T20:23:00Z

I pushed the same changes again and it passed.
Please review the changes.

sympy/solvers/solvers.py

oscarbenjamin · 2020-03-10T00:27:26Z

I think translating binomials should still come later after the loop that it is in now.

This will need to have a test added for equations with binomial coefficients that can be solved.

sbt4104 · 2020-03-10T03:27:48Z

I will make the changes.

sbt4104 · 2020-03-10T06:25:47Z

I have added the test.

sbt4104 · 2020-03-10T17:26:10Z

I have added the binomial expansion after the loop, please suggest changes.

sympy/solvers/solvers.py

oscarbenjamin · 2020-03-11T11:50:41Z

It is failing if in binomial(n,k) n is a polynomial in x, example
y = solve(1-binomial(x+1, 3),x)

In what way was it failing?

sbt4104 · 2020-03-11T12:29:12Z

I had written it like this in the previous commit
f = f.replace(binomial, lambda n, k: expand_func(binomial(n, k)) if n==symbol else binomial(n, k))
so for y = solve(1-binomial(x+1, 3),x), it was comparing x+1 with x and returned 'NotImplementedError:
No algorithms are implemented to solve equation binomial(x + 3, 3)'

though it worked perfectly for y = solve(1-binomial(x+1, 3), x+1)

so i changed it to solve cases like this
f = f.replace(binomial, lambda n, k: expand_func(binomial(n, k)) if symbol in n.free_symbols and len(n.free_symbols) == 1 else binomial(n,k))

oscarbenjamin · 2020-03-11T12:57:37Z

This is probably best:

In [5]: syms = [x]                                                                                                                                            

In [6]: binomial(x, 2).replace(lambda e: isinstance(e, binomial) and e.has(*syms), lambda e: expand_func(e))                                                  
Out[6]: 
x⋅(x - 1)
─────────
    2

oscarbenjamin · 2020-03-11T12:58:52Z

It looks like expand_func for binomial could be improved:

In [18]: expand_func(binomial(x, 2))                                                                                                                          
Out[18]: 
x⋅(x - 1)
─────────
    2    

In [19]: expand_func(binomial(x+2, x))                                                                                                                        
Out[19]: 
⎛x + 2⎞
⎜     ⎟
⎝  x  ⎠

In [20]: gammasimp(_.rewrite(factorial))                                                                                                                      
Out[20]: 
(x + 1)⋅(x + 2)
───────────────
       2

sbt4104 · 2020-03-11T13:32:49Z

This is probably best:

In [5]: syms = [x]                                                                                                                                            

In [6]: binomial(x, 2).replace(lambda e: isinstance(e, binomial) and e.has(*syms), lambda e: expand_func(e))                                                  
Out[6]: 
x⋅(x - 1)
─────────
    2

Should we expand the binomial if we have a multivariable equation, example
binomial(x + y, 2)

oscarbenjamin · 2020-03-11T13:51:38Z

Should we expand the binomial if we have a multivariable equation

Yes if any of the symbols of interest is in the binomial. Otherwise we won't be able to solve the equations.

sbt4104 · 2020-03-11T13:58:55Z

ok, I will make that change.

sbt4104 · 2020-03-11T15:30:50Z

I have made the changes, please review.

sbt4104 · 2020-03-11T15:36:30Z

It looks like expand_func for binomial could be improved:

In [18]: expand_func(binomial(x, 2))                                                                                                                          
Out[18]: 
x⋅(x - 1)
─────────
    2    

In [19]: expand_func(binomial(x+2, x))                                                                                                                        
Out[19]: 
⎛x + 2⎞
⎜     ⎟
⎝  x  ⎠

In [20]: gammasimp(_.rewrite(factorial))                                                                                                                      
Out[20]: 
(x + 1)⋅(x + 2)
───────────────
       2

expand_func expands binomial(x, x-1) properly but fails for binomial(x+1, x)

I think changing
if k.is_Add and n in k.args : k = n - k
to
if (k.is_Add and n in k.args) or (n.is_Add and k in n.args) : k = n - k
will solve this problem

oscarbenjamin · 2020-03-11T17:06:50Z

Or maybe just if (k - n).is_Integer

sbt4104 · 2020-03-11T17:17:28Z

yes, this one will also expand cases like
solve_func(binomial(x + 1, x - 1))

sbt4104 · 2020-03-11T17:18:29Z

should I send a PR for this along with the issue?

oscarbenjamin · 2020-03-11T17:37:33Z

You can make those changes in this PR as well

sbt4104 · 2020-03-11T19:29:47Z

I have made the changes, please review.

oscarbenjamin · 2020-03-11T20:08:56Z

sympy/functions/combinatorial/factorials.py

@@ -1012,7 +1012,7 @@ def _eval_expand_func(self, **hints):
            return binomial(*self.args)

        k = self.args[1]
-        if k.is_Add and n in k.args:
+        if (n-k).is_Integer:
            k = n - k


There should be a test for this.

oscarbenjamin · 2020-03-11T20:09:14Z

sympy/solvers/solvers.py

@@ -1460,6 +1460,9 @@ def _solve(f, *symbols, **flags):
            raise NotImplementedError(not_impl_msg % f)
    symbol = symbols[0]

+    #expand binomials only if it has the unknown symbol
+    f = f.replace(lambda e: isinstance(e, binomial) and e.has(symbol), lambda e: expand_func(e))


This line is too long

oscarbenjamin · 2020-03-11T20:09:33Z

sympy/solvers/tests/test_solvers.py

+    assert solve(Eq(binomial(x,2),0)) == [0, 1]
+    assert solve(a+binomial(x, 3),a) == [-binomial(x, 3)]
+    assert solve(x-binomial(a, 3) + binomial(y,2) + sin(a),x) == [-sin(a) + binomial(a, 3) - binomial(y, 2)]
+    assert solve((x+1)-binomial(x+1, 3),x) == [-2, -1, 3]


Include a space after a comma

oscarbenjamin · 2020-03-11T22:31:29Z

This looks good. I'll merge it once tests pass.

For future PRs can you try to use better commit messages? The messages here started out good but then got less good as time went on. You should imagine that the message will be looked at in future by someone who does not know the full context of the PR. Something like "add test" does not convey much about the change that is being made to the code.

sbt4104 · 2020-03-11T22:53:20Z

I will keep it in mind, this will not happen again.

oscarbenjamin · 2020-03-11T23:28:37Z

Merged. Thanks for this!

sbt4104 · 2020-03-12T15:50:13Z

Thank you, l learned a lot from this.

Update solvers to handle binomial.

dde2e07

Update solvers to handle binomials.

836c5c5

Place condition in preprocess equation.

132a7b6

Place condition in preprocess equation(remove whitespaces).

0a6be89

sbt4104 added 2 commits March 9, 2020 20:09

changed solver to expand only binomials from expression.

f06a5dc

changed solver to expand only binomials from expression.

e828057

whitespaces removed.

f912814

changed solver to expand only binomials from expression.

2c90166

czgdp1807 added the solvers label Mar 9, 2020

oscarbenjamin reviewed Mar 9, 2020

View reviewed changes

sympy/solvers/solvers.py Outdated Show resolved Hide resolved

changed filter to identify only binomial.

fa80f84

sbt4104 requested a review from oscarbenjamin March 9, 2020 23:21

Add test.

cf7e36a

oscarbenjamin reviewed Mar 10, 2020

View reviewed changes

sympy/solvers/solvers.py Outdated Show resolved Hide resolved

sbt4104 added 2 commits March 10, 2020 23:04

Update solver to handle binomials.

8c69f93

Update solvers.

6f10d0a

sbt4104 requested a review from oscarbenjamin March 10, 2020 20:11

add tests

5ffb685

expand binomial having unknown symbol.

66a43b9

Update expand for binomials.

6bd1288

oscarbenjamin reviewed Mar 11, 2020

View reviewed changes

sbt4104 added 4 commits March 12, 2020 01:44

Proper spaces after comma.

8b1f1d6

add test

35d13b1

add test.

16481fa

add tests.

bdb47f3

oscarbenjamin merged commit 516fa83 into sympy:master Mar 11, 2020

sbt4104 deleted the update_solver_to_handle_binomial branch March 12, 2020 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update solvers to handle binomial. #18802

Update solvers to handle binomial. #18802

sbt4104 commented Mar 8, 2020 •

edited

sympy-bot commented Mar 8, 2020 •

edited

oscarbenjamin commented Mar 8, 2020

sbt4104 commented Mar 8, 2020 •

edited

codecov bot commented Mar 8, 2020 •

edited

sbt4104 commented Mar 9, 2020

oscarbenjamin commented Mar 9, 2020

sbt4104 commented Mar 9, 2020

sbt4104 commented Mar 9, 2020

sbt4104 commented Mar 9, 2020

oscarbenjamin commented Mar 10, 2020

sbt4104 commented Mar 10, 2020

sbt4104 commented Mar 10, 2020

sbt4104 commented Mar 10, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin Mar 11, 2020

oscarbenjamin Mar 11, 2020

oscarbenjamin Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 12, 2020

Update solvers to handle binomial. #18802

Update solvers to handle binomial. #18802

Conversation

sbt4104 commented Mar 8, 2020 • edited

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Mar 8, 2020 • edited

oscarbenjamin commented Mar 8, 2020

sbt4104 commented Mar 8, 2020 • edited

codecov bot commented Mar 8, 2020 • edited

Codecov Report

sbt4104 commented Mar 9, 2020

oscarbenjamin commented Mar 9, 2020

sbt4104 commented Mar 9, 2020

sbt4104 commented Mar 9, 2020

sbt4104 commented Mar 9, 2020

oscarbenjamin commented Mar 10, 2020

sbt4104 commented Mar 10, 2020

sbt4104 commented Mar 10, 2020

sbt4104 commented Mar 10, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin Mar 11, 2020

Choose a reason for hiding this comment

oscarbenjamin Mar 11, 2020

Choose a reason for hiding this comment

oscarbenjamin Mar 11, 2020

Choose a reason for hiding this comment

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 11, 2020

oscarbenjamin commented Mar 11, 2020

sbt4104 commented Mar 12, 2020

sbt4104 commented Mar 8, 2020 •

edited

sympy-bot commented Mar 8, 2020 •

edited

sbt4104 commented Mar 8, 2020 •

edited

codecov bot commented Mar 8, 2020 •

edited