-
-
Notifications
You must be signed in to change notification settings - Fork 62
Fix Gamma[] simplification rules.
#1387
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
mathics/builtin/specialfns/gamma.py
Outdated
| rules = { | ||
| "Gamma[z_, x0_, x1_]": "Gamma[z, x0] - Gamma[z, x1]", | ||
| "Gamma[1 + z_]": "z!", | ||
| "Gamma[1 + z_?IntegerQ]": "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.
| "Gamma[1 + z_?IntegerQ]": "z!", | |
| "Gamma[1 + z_?NumberQ]": "z!", |
Factorial is defined for numbers outside of integers. For example 2.5! is 3.32335.
For those numbers like E, Pi, and I where factorial is not defined, Mathics3 Factorial returns the expression unevaluated without an error; and this is the desired behavior we want in this case.
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.
Hello, @rocky. This should be _?IntegerQ because for non-integer x factorial is just defined as Gamma[1+x] so there's no point converting back and forth... Whereas for integers, notation-wise, the having x! looks good...
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.
MMA, e.g., follows this.
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.
Hello, @rocky. This should be
_?IntegerQbecause for non-integerxfactorial is just defined asGamma[1+x]so there's no point converting back and forth... Whereas for integers, notation-wise, the havingx!looks good...
I just pulled the branch and tried Gamma[1 + 2.5] and this code works. So I don't have a strong opinion one way or the other.
But from a consistency standpoint, if x! looks good for integer x, it should also look good for real x as well in my opinion. Or remove both. It is just that having it go one way for integers and the other way for reals seems weird to me.
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.
Hello, @rocky. This should be
_?IntegerQbecause for non-integerxfactorial is just defined asGamma[1+x]so there's no point converting back and forth... Whereas for integers, notation-wise, the havingx!looks good...
One things I was going to bring up but didn't because I didn't think were were going to go at the end with an integer test: the preferred way to express this is more simply: _Integer. When what you are testing is defined type, you can drop the "?".
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.
As you're aware, factorial was first defined for positive integers, and then analytically continued to non-integers and complex numbers via the Gamma function. To me, it feels strange to see x! notation for non-integral x. Also, MMA does not perform this simplification for arbitrary x.
In[7]:= FullSimplify[Gamma[1+x]]
Out[7]= Gamma[1+x]
In[8]:= FullSimplify[Gamma[1+x], Element[x, Integers] && x > 0]
Out[8]= x!But, I will leave this open and wait for others to provide an opinion as well.
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 - thanks for the further explanation. There is a lot of leeway for matters of taste. And say you say, WMA does not FuillSimplify with Gamma to factorial when Integer is replaced with Rationals or Reals.
So aside from the fact that there is a failing test, I am fine with this. @mmatera ?
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.
So aside from the fact that there is a failing test, I am fine with this. @mmatera ?
@rocky on my end all tests are passing:
================== 2870 passed, 364 skipped, 14 xfailed, 16 xpassed, 3 warnings in 131.40s (0:02:11) ===================Could you please let me know which test is failing? Perhaps I can fix it if it's related to this change...
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.
Look at the actions tab. See for example: https://github.com/Mathics3/mathics-core/actions/runs/13345211275/job/37274817229?pr=1387
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.
Look at the actions tab. See for example: https://github.com/Mathics3/mathics-core/actions/runs/13345211275/job/37274817229?pr=1387
This is a problem because MMA evaluates it to a factorial. I'll take a deeper look...
Actually, according to WMA and the example Previously, we had discussed how this could be expanded to Numbers rather than Integers. But WMA behavior takes this to be rewritten to be valid no matter what |
In[7]:= FullSimplify[Gamma[1+x]]
Out[7]= Gamma[1+x]
In[8]:= FullSimplify[Gamma[1+x], Element[x, Integers] && x > 0]
Out[8]= x!@rocky as I showed you in the comment before, |
I don't know what comment you are referring to. We are coming to see that what is done in
The back and forth is because we have new information. When information changes, assessments might change. A test that someone wrote to match a particular (WMA) behavior started failing. In investigating that, I see that things are a little bit different than I had understood before. See below.
It may be strange, but factorial is defined for non-integral x: And Concrete Mathmatics by Graham, Knuth and Patashnik (the first reference in the Wiki page on factorial) defines factorial using the postfix operator "!" for non-integer numbers this way too.
Gamma and Factorial are related. The latter is the more specialized function though, and specialized functions are used when they apply.
We've come across situations where WMA does things differently from convention; it might even be that WMA in some situations has changed conventional mathematics expectations, (but I don't think that is the case here). In any event, in the past what we have invariably done is go with WMA's behavior rather than what we feel is right. Of course, there is some leeway in how to implement that behavior, but it seems to me that right now |
|
@rocky Unfortunately, (* Inconsistent *)
In[1]:= Gamma[1+x]
Out[1]= x!
(* Consistent *)
In[2]:= Product[k, {k, 3, n}]
Out[2]= n! / 2
(* Not (x+1)! *)
In[3]:= Gamma[2+x]
Out[3]= Gamma[2 + x]I'd still vote not to have the generic rule |
|
I was not follow this discussion very closely, but as a general rule, except when there is a very good reason, we go for the most compatible behavior. |
I certainly don't want to make day-to-day work with Mathics more difficult. I would appreciate it if you would give specific details here. Here is a recap of where things stand. This PR makes the expression I'd also like to note that removing the rule If we want to match WMA for both Here is a trace of an evaluation for this with SymPy tracing on: We might be able to detect the Factorial via |
|
#1395 does this better... |
We want to match the behavior of WMA for `Gamma` and `Product`.
Specifically:
```
Product[k, {k, 3, n}] == n! / 2
Gamma[1+x] == Gamma[1+x]
```
Supercedes #1387

Introduction
Gamma[]function has the rule:However, this rule is only valid when
zis an integer.The fix changes the rule to apply only to integral
z.Test
Compare
Gamma[1+x]between MMA and Mathics.