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.

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

[GSoC] Functions: Fixes errors in assumptions when rewriting RisingFactorial / FallingFactorial as gamma or factorial

Merged
merged 14 commits into from Jul 12, 2020

Conversation

sachin-4099
Copy link
Contributor

@sachin-4099 sachin-4099 commented Feb 20, 2020

Fixes: #6878
Fixes: #13446
Fixes: #14871
Fixes: #18686

Brief description of what is fixed or changed

Previously:

In[1]: ff(5,y).rewrite(gamma)
Out[1]: 0  #Wrong Answer

Now:

In[1]: ff(5,y).rewrite(gamma)
Out[1]: 120/Gamma(6 - y)  #Correct Answer

Other Comments

Regression Tests have been added

Release Notes

  • functions
    • fixed errors in assumptions when rewriting RisingFactorial / FallingFactorial as gamma or factorial

@sympy-bot
Copy link

sympy-bot commented Feb 20, 2020

Hi, I am the SymPy bot (v160). 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
    • fixed errors in assumptions when rewriting RisingFactorial / FallingFactorial as gamma or factorial (#18696 by @sachin-4099)

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

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.

Fixes: #6878
Fixes: #13446
Fixes: #14871
Fixes: #18686 


#### Brief description of what is fixed or changed
**Previously**: 
```
In[1]: ff(5,y).rewrite(gamma)
Out[1]: 0  #Wrong Answer
```

**Now**: 
```
In[1]: ff(5,y).rewrite(gamma)
Out[1]: 120/Gamma(6 - y)  #Correct Answer
```




#### Other Comments
Regression Tests have been added


#### Release Notes


<!-- BEGIN RELEASE NOTES -->
* functions
  * fixed errors in assumptions when rewriting `RisingFactorial` / `FallingFactorial` as `gamma` or `factorial`
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@smichr
Copy link
Member

smichr commented Feb 20, 2020

I suggest that you use as a starting point my ffrewrite branch. Just pull that and replace your ff_rewrite with it and make modifications and push here with the -f option and it will update this PR.

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Feb 20, 2020

I suggest that you use as a starting point my ffrewrite branch. Just pull that and replace your ff_rewrite with it and make modifications and push here with the -f option and it will update this PR.

Extremely sorry I read late

@smichr
Copy link
Member

smichr commented Feb 20, 2020

Extremely sorry I read late

No problem...and I just got a better Piecewise, I think. See how it goes as you have time.

@smichr
Copy link
Member

smichr commented Feb 21, 2020

A diff to update this with changes I had in progress is here

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Feb 21, 2020

A diff to update this with changes I had in progress is here

Doctests as well as tests in concrete module are failing with this diff

@smichr
Copy link
Member

smichr commented Feb 21, 2020

The is_hypergeometric function has to be made to look for a Piecewise and return True if all the expressions in the Piecewise are hypergeometric. But better, Piecewise should be able to respond to all is_what requests by returning a value based on all its args meeting that criteria.

Piecewise already knows how to handle a rewrite request (e.g. Piecewise((factorial(x),x>0),(1,True)).rewite(gamma) works. I believe the core.basic.rewrite and core.basic._eval_rewrite are able to work out how to rewrite the args of Piecewise without Piecewise having all _eval_rewrite_as_what. It would be good if we could do the same thing for all is_what properties.

@jksuom
Copy link
Member

jksuom commented Feb 21, 2020

This is the critical line in hypersimp:

g = g.rewrite(gamma)

It creates a Piecewise function that the following lines cannot currently simplify. It would suffice to consider the rightmost interval in piecewise. In some cases, this can be enforced by adding assumptions to arguments.

In [1]: n, k = symbols('n k', positive=True)

In [2]: ff(n, k).is_hypergeometric(n)
Out[2]: True

In [3]: ff(n, k).is_hypergeometric(k)
Out[3]: True

In [4]: ff(n-1, k+1).is_hypergeometric(n)
Out[4]: False

@jksuom
Copy link
Member

jksuom commented Feb 21, 2020

return True if all the expressions in the Piecewise are hypergeometric

There is a problem with this: The input expression has no Piecewise. It only appears in the process, in hypersimp that is called by is_hypergeometric. Besides, it is not necessary that all pieces of a Piecewise be hypergeometric, it would suffice that the condition will hold on some infinite interval extending to oo. I would consider defining a function piecewise_select(k) that would substitute k = oo in the conditions and pick the expression that corresponds to the first condition that evaluates to True. Then hypersimp would continue with that expression.

@smichr
Copy link
Member

smichr commented Feb 21, 2020

n, k = symbols('n k', positive=True)

this is the simplest fix but these functions have meaning for non-integer cases so I think we need a more robust solution. Or, for the sake of this PR, should we just do that and deal with how to handle Piecewise in another PR?

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Feb 21, 2020

I will do as you say, IMHO, we can currently do this and then it is better to handle the remaining work in another PR.

@smichr
Copy link
Member

smichr commented Feb 21, 2020

gitter is not working for me.

Regarding how to proceed, I would like to hear from @jksuom and/or @oscarbenjamin . If we commit this without the improved handling in place then anyone relying on the current behavior will have a broken system.

@jksuom
Copy link
Member

jksuom commented Feb 21, 2020

I think that there are two possibilities.

rewrite could return a Piecewise expression and hypersimp would handle them by selecting one of the expressions. In principle, it should not matter which one as they are all supposed to represent the same function. However, the interval extending to oo seems the most logical choice as the property is used for series with upper limit oo.

Alternatively, rewrite could select one of the expressions on basis of what is known of the arguments.

⎧   Γ(x + 1)               
⎪ ────────────    if x not a negative integer
⎪ Γ(x - y + 1)             
⎪                          
⎨    y                     
⎪(-1) ⋅Γ(-x + y)           
⎪───────────────  otherwise
⎪     Γ(-x)                
⎩                          

@smichr
Copy link
Member

smichr commented Feb 21, 2020

Alternatively, rewrite could select one of the expressions on basis of what is known of the arguments.

If I understand correctly, this is what I was doing in my ffrewrite branch which I referenced as a diff for this PR here. But I check for negative or non-integer args.

@jksuom
Copy link
Member

jksuom commented Feb 21, 2020

I mean something like this:

if x.is_negative and x.is_integer:
    return (-1)**k*gamma(k - x)/gamma(-x)
else:
    return gamma(x + 1)/gamma(-k + x + 1)

@smichr
Copy link
Member

smichr commented Feb 21, 2020

I mean something like this:

The problem with this is that if it is not known then then else-form will be returned but then it will evaluate incorrectly when given a negative argument. That is what the Piecewise was trying to circumvent.

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Feb 22, 2020

I think that using a Piecewise is the right approach in general. I don't understand what problem it is causing though... (probably too tired)

@smichr
Copy link
Member

smichr commented Feb 22, 2020

I don't understand what problem it is causing

It fails Piecewise(...).is_hypergeometric.

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Feb 22, 2020

I don't understand what problem it is causing

It fails Piecewise(...).is_hypergeometric.

Should that pass though?

In [14]: S(1).is_hypergeometric(x)                                                                                                             
Out[14]: True

In [15]: S(2).is_hypergeometric(x)                                                                                                             
Out[15]: True

In [16]: Piecewise((1, x>0), (2, True)).is_hypergeometric(x)                                                                                   
Out[16]: False

That Piecewise not a hypergeometric function because it can not be represented using any hypergeometric power series.

What is the example function for which this fails and why is it important to know if it is actually hypergeometric?

@smichr
Copy link
Member

smichr commented Feb 22, 2020

What is the example function

>>> rf(x,y).is_hypergeometric(x)
True
>>> rf(x,y).is_hypergeometric(y)
True

>>> (x+y).is_hypergeometric(x)
True
>>> (gamma(x + y)/gamma(x)).is_hypergeometric(x)
True
>>> Piecewise((gamma(x + y)/gamma(x),x>=0),(x+y,True)).is_hypergeometric(x)
False

(That's just a toy representation of the idea; and if I am repeating something that doesn't make sense in light of what @jksuom has already told me it is a reflection of my almost total ignorance about hypergeometricity)

@jksuom
Copy link
Member

jksuom commented Feb 22, 2020

it will evaluate incorrectly when given a negative argument.

There are two operations, rewriting and giving an argument. Would it be possible to give the argument before rewriting?

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Feb 22, 2020

If rf(x, y) is hypergeometric in x then I guess the corresponding Piecewise should be. I don't know how sympy could come to that conclusion though apart from realising that the Piecewise is equivalent to rf(x, y).

I don't think that a Piecewise is hypergeometric just because each of its cases would be.

@smichr
Copy link
Member

smichr commented Feb 22, 2020

There are two operations, rewriting and giving an argument. Would it be possible to give the argument before rewriting?

In the failing doctests an assertion that rf and ff are hypergeometric fails because the method tries to simplify the input -- the rewrite(gamma) step. Can that routine make the determination before rewriting as gamma? Like, is any linear combination of rf or ff hypergeometric?

Perhaps rewrite should simply fail if the assumption is not present (thus forcing the user to give the argument before rewriting). But a rewrite(Piecewise) could be defined to rewrite in terms of factorial and then, if the person wanted, it could be rewritten in terms of gamma.

So rf(pos int, int).rewrite(gamma) would succeed while rf(x,y).rewrite(gamma) would fail to change.

@czgdp1807
Copy link
Member

czgdp1807 commented May 5, 2020

Any news on this? @sachin-4099 @smichr

@czgdp1807 czgdp1807 added the GSoC label May 5, 2020
@sachin-4099
Copy link
Contributor Author

sachin-4099 commented May 5, 2020

Any news on this? @sachin-4099 @smichr

Thanks for reminding, I will look into this and finish this.

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jul 6, 2020

Can a piecewise be hypergeometric in certain cases?? @oscarbenjamin @smichr

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jul 6, 2020

Can a piecewise be hypergeometric in certain cases?? @oscarbenjamin @smichr

I don't see why not. Note that the Piecewise can have conditions that don't depend on the variable of interest e.g.:

In [8]: expr = x * Piecewise((1, y<0), (2, True))                                                                                              

In [9]: expr                                                                                                                                   
Out[9]: 
  ⎛⎧1  for y < 0⎞
x⎜⎨            ⎟
  ⎝⎩2  otherwise⎠

In [10]: expr.is_hypergeometric(x)                                                                                                             
Out[10]: True

In [11]: expr.is_hypergeometric(y)                                                                                                             
Out[11]: False

In general a piecewise function of x can be at least piecewise-hypergeometric (if each piece is hypergeometric).

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jul 6, 2020

So, in your opinion, this is not correct:

    def is_hypergeometric(self, k):
        from sympy.simplify import hypersimp
        from sympy.functions import Piecewise
        if self.has(Piecewise):
            return None
        return hypersimp(self, k) is not None

@oscarbenjamin
Copy link
Contributor

oscarbenjamin commented Jul 6, 2020

None is always a valid answer in fuzzy logic.

@sachin-4099 sachin-4099 requested review from smichr and removed request for smichr Jul 7, 2020
@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jul 7, 2020

@smichr are the changes of this diff handled in this PR: #18960 ??

sympy/functions/combinatorial/factorials.py Outdated Show resolved Hide resolved
@iammosespaulr
Copy link
Contributor

iammosespaulr commented Jul 8, 2020

@smichr are the changes of this diff handled in this PR: #18960 ??

No not in 18960.
I planned on taking a different approach in #19383

but I think radical changes here would have unintended consequences w.r.t the assumptions for #18960 .

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jul 8, 2020

Since, k is an integer these are the required changes, it would be good if you can come up with an alternative suggestion to handle the situation. I don't think that we have made any changes which are incorrect.
I guess some more changes need to be done in some testcases, because those testcases were using the previous rewrite which was wrong for negative x.

but I think radical changes here would have unintended consequences w.r.t the assumptions for #18960 .

Can you elaborate on this?

@iammosespaulr
Copy link
Contributor

iammosespaulr commented Jul 8, 2020

Can you elaborate on this?

You're returning a Piecewise function while rewriting as gamma.
is it worth making a lot of changes to the existing codebase to accommodate this change ?

maybe I'm the only one who feels this way. Why use Piecewise when there are alternatives.

I guess if @oscarbenjamin @jksuom @smichr are okay with the approach then it's cool.

@sachin-4099
Copy link
Contributor Author

sachin-4099 commented Jul 9, 2020

Why use Piecewise when there are alternatives.

What are the alternatives, can you elaborate keeping in mind the issue #13446?
How can we come up with a general rewrite if the the arguments of rf or ff are just symbols x and k without any assumptions??

@sachin-4099 sachin-4099 changed the title Functions: FallingFactorial rewrite to gamma error resolved [GSoC] Functions: Fixes errors in assumptions when rewriting RisingFactorial / FallingFactorial as gamma Jul 9, 2020
@sachin-4099 sachin-4099 changed the title [GSoC] Functions: Fixes errors in assumptions when rewriting RisingFactorial / FallingFactorial as gamma [GSoC] Functions: Fixes errors in assumptions when rewriting RisingFactorial / FallingFactorial as gamma or factorial Jul 9, 2020
sympy/concrete/products.py Outdated Show resolved Hide resolved
@sachin-4099 sachin-4099 requested review from smichr and removed request for smichr Jul 11, 2020
@sachin-4099 sachin-4099 force-pushed the ff_rewrite branch 2 times, most recently from c6222e9 to 2025dda Compare Jul 11, 2020
@jksuom
Copy link
Member

jksuom commented Jul 12, 2020

Thanks, this is ready to be merged.

@jksuom jksuom merged commit 1eaefeb into sympy:master Jul 12, 2020
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment