-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Changes from all commits
88f6d61
36f8dba
daaec23
e0057f2
25bfeee
eff651f
e432f5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -336,7 +336,6 @@ class Gamma(MPMathMultiFunction): | |||||
|
|
||||||
| rules = { | ||||||
| "Gamma[z_, x0_, x1_]": "Gamma[z, x0] - Gamma[z, x1]", | ||||||
| "Gamma[1 + z_]": "z!", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rule could be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
| "Gamma[Undefined]": "Undefined", | ||||||
| "Gamma[x_, Undefined]": "Undefined", | ||||||
| "Gamma[Undefined, y_]": "Undefined", | ||||||
|
|
@@ -452,7 +451,16 @@ class Pochhammer(SympyFunction): | |||||
|
|
||||||
| rules = { | ||||||
| "Pochhammer[0, 0]": "1", | ||||||
| "Pochhammer[a_, n_]": "Gamma[a + n] / Gamma[a]", | ||||||
| # FIXME: In WMA, if n is an Number with an integer value, it | ||||||
| # is rewritten to expanded terms not using | ||||||
| # Factorial as we do below. For example, Pochhammer[i, 2] is | ||||||
| # i (i + 1) instead of (1 + i)! / (-1 + i)! as the rule below | ||||||
| # gives. Ideally, we should match this behavior. However if | ||||||
| # this is done, we will *also* need to adjust Product, because | ||||||
| # WMA Product is sometimes rewritten as an expression using Factorial. | ||||||
| # In particular, Product[k, {k, 3, n}] == n! / 2 in both Mathics3 | ||||||
| # and WMA. | ||||||
| "Pochhammer[a_, n_]": "Factorial[a + n - 1] / Factorial[a - 1]", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what Notice that
Suggested change
or maybe
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the examples cited,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||||||
| "Derivative[1,0][Pochhammer]": "(Pochhammer[#1, #2]*(-PolyGamma[0, #1] + PolyGamma[0, #1 + #2]))&", | ||||||
| "Derivative[0,1][Pochhammer]": "(Pochhammer[#1, #2]*PolyGamma[0, #1 + #2])&", | ||||||
| } | ||||||
|
|
||||||
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.