- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 61
 
          Match WMA for Gamma[1+x] and Product[...]
          #1395
        
          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
Conversation
| rules = { | ||
| "Pochhammer[0, 0]": "1", | ||
| "Pochhammer[a_, n_]": "Gamma[a + n] / Gamma[a]", | ||
| "Pochhammer[a_, n_]": "Factorial[a + n - 1] / Factorial[a - 1]", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what Pochhammer should do:
In[1]:= Pochhammer[a,b]                                                         
Out[1]= Pochhammer[a, b]
In[2]:= Pochhammer[a,12.4]                                                      
Out[2]= Pochhammer[a, 12.4]
In[3]:= Pochhammer[a,12.]                                                       
Out[3]= a (1 + a) (2 + a) (3 + a) (4 + a) (5 + a) (6 + a) (7 + a) (8 + a) 
 
>    (9 + a) (10 + a) (11 + a)
In[4]:= Pochhammer[12,b]                                                        
Out[4]= Pochhammer[12, b]
In[5]:= Pochhammer[12.1,b]                                                      
Out[5]= Pochhammer[12.1, b]
Notice that n must be an Integer to apply this rule.
| "Pochhammer[a_, n_]": "Factorial[a + n - 1] / Factorial[a - 1]", | |
| "Pochhammer[a_, n_Integer]": "Factorial[a + n - 1] / Factorial[a - 1]", | 
or maybe n_?NumerQ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the examples cited, NumberQ  only works when the number can be exactly converted to an Integer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. In any case, this is the point when we need to decide if the complexity of a rule that matches the exact behavior worth. The exact behavior would be obtained with something like
        "Pochhammer[a_, /;(IntegerQ[n]|| NumberQ[n] && Round[n]==n)]": "Factorial[a + n - 1] / Factorial[a - 1]",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IntegerQ[n] can be dropped following the principle: simple as possible....  But even if we change to Pochhammer[a_, n_(NumberQ[n] && Round[n]==n)], Product[k, {k, i, n}]  will not transform as WMA does, in particular:  n! / (-1+i)!.   So WMA may be using some other mechanism to implement Product.
But let's back up a bit. We got into this as a result of a real problem reported by @aravindh-krishnamoorthy, in his day-to-day use of Mathics3. His PR broke something else. If this PR addresses both his problem and does not break what is already there, then let's go with this.
Note that this PR adds a test to ensure we do not rewrite Gamma as Factorial in the kind of way that @aravindh-krishnamoorthy was not expecting.
In the future, if someone has a problem as a result of Pochhammer getting expanded to factorial, then we can address that issue at that time while maintaining satisfaction of all the other checks in place.
In other words: complicate when there is a tangible need to complicate. Again this is also encompassed by the principle: Simple as possible, but no simpler.
| 
               | 
          ||
| rules = { | ||
| "Gamma[z_, x0_, x1_]": "Gamma[z, x0] - Gamma[z, x1]", | ||
| "Gamma[1 + z_]": "z!", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule could be
"Gamma[z_Integer]": "(z-1)!"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this cause any observable difference in output? If not, I'd recommend removing the rule unless there is a good reason to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have different implementations for Gamma and Factorial, relying on different mpmath functions. Right now, mpmath function gives the same answer, so both outputs are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great - then let's not complicate here when there is no user-observable difference.
| summary_text = "compute the complete and incomplete gamma functions" | ||
| 
               | 
          ||
| rules = { | ||
| "Gamma[z_, x0_, x1_]": "Gamma[z, x0] - Gamma[z, x1]", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In WMA, this rule only applies if the three arguments are numeric, and at least one is a Real number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine then that this kind of check would be better done inside an evaluation method.
If you are up for adding, feel free to do. But if not, let's leave for another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's postpone for another PR.
01145c5    to
    e0057f2      
    Compare
  
    and go over doc text
c1c065a    to
    25bfeee      
    Compare
  
    | 
           I'll review this tomorrow  | 
    
| 
           
 As @mmatera suggests, integer checks for   | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| 
           Thanks everyone for looking at this. If anyone had an apparent and straightforward fix, we would have taken it. Checking compatibility was a good idea, and I will note this information in a comment before committing, so we can fix it later. But the situation, as best as I can tell, is that although there are ideas for how we might address this, we have nothing concrete. And some of these feel a bit involved. So that is why I say we stop here and leave the rest for later when needed.  | 
    
In the future, Pochhamer may be adjusted to match WMA behavior better. If and when that happens, Product may need to be adjusted to use Factorial.
debf9f5    to
    e432f5c      
    Compare
  
    Gamma[1+x] and Product[...]
      
We want to match the behavior of WMA for
GammaandProduct. Specifically:Supercedes #1387