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
Added multigamma function #16843
Added multigamma function #16843
Conversation
✅ Hi, I am the SymPy bot (v147). 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:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5. 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.
Update The release notes on the wiki have been updated. |
@sidhantnagpal this is a new PR since I did a blunder with older PR. Please review this. |
raise TypeError('p must be a positive integer') | ||
from sympy import Product | ||
from sympy.abc import k | ||
return pi**(psym*(psym-1)/4)*Product(gamma(x + (1 - k)/2), (k, 1, psym)) |
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.
Lower case methods return evaluated results when possible, that is they shouldn't require doit
.
That said, unevaluated expressions can still be obtained by passing evaluate=False
.
>>> gamma(5)
24
>>> gamma(S(5)/2)
3*sqrt(pi)/4
>>> gamma(7, evaluate=False)
gamma(7)
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.
What do think about the following diff:
--- a/sympy/functions/special/gamma_functions.py
+++ b/sympy/functions/special/gamma_functions.py
@@ -1010,7 +1010,7 @@ def trigamma(x):
"""
return polygamma(1, x)
-def multivariate_gamma(x, p):
+def multivariate_gamma(x, p, evaluate=True):
r"""
The multivariate gamma function is a generalization of the gamma function i.e,
@@ -1065,8 +1065,11 @@ def multivariate_gamma(x, p):
.. [1] https://en.wikipedia.org/wiki/Multivariate_gamma_function
"""
psym = sympify(p)
- if fuzzy_not(fuzzy_and([psym.is_integer, psym.is_positive])):
+ if psym.is_integer is False or psym.is_positive is False:
raise TypeError('p must be a positive integer')
from sympy import Product
from sympy.abc import k
+ if evaluate:
+ return (pi**(psym*(psym-1)/4)*Product(gamma(x + (1 - k)/2),
+ (k, 1, psym))).doit()
return pi**(psym*(psym-1)/4)*Product(gamma(x + (1 - k)/2), (k, 1, psym))
Example usage:
>>> from sympy import *
>>> x, p = symbols('x p')
>>> mg = multivariate_gamma
>>> mg(1, 1)
1
>>> mg(1, 1, evaluate=False)
Product(gamma(3/2 - k/2), (k, 1, 1))
>>> mg(x, 1)
gamma(x)
>>> mg(x, 1, evaluate=False)
Product(gamma(-k/2 + x + 1/2), (k, 1, 1))
>>> mg(x, p)
pi**(p*(p - 1)/4)*Product(gamma(-k/2 + x + 1/2), (k, 1, p))
>>> mg(1, p)
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 think multivariate_gamma
could be implemented along the lines of gamma
inheriting from Function
(and not digamma
or trigamma
which are just Python functions based off polygamma
).
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.
@sidhantnagpal I am trying to implement multivariate gamma function along the lines of gamma
class. I found the following bug with gamma function.
>>> gamma(-1) # -1 is pole for gamma function
zoo
>>> gamma(-1.0)
Traceback (most recent call last):
File "/home/ritesh/Desktop/sympy/sympy/core/evalf.py", line 1308, in evalf
rf = evalf_table[x.func]
KeyError: gamma
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/ritesh/Desktop/sympy/sympy/core/cache.py", line 94, in wrapper
retval = cfunc(*args, **kwargs)
File "/home/ritesh/Desktop/sympy/sympy/core/function.py", line 463, in __new__
result = result.evalf(mlib.libmpf.prec_to_dps(pr))
File "/home/ritesh/Desktop/sympy/sympy/core/evalf.py", line 1442, in evalf
result = evalf(self, prec + 4, options)
File "/home/ritesh/Desktop/sympy/sympy/core/evalf.py", line 1314, in evalf
xe = x._eval_evalf(prec)
File "/home/ritesh/Desktop/sympy/sympy/core/function.py", line 586, in _eval_evalf
v = func(*args)
File "/home/ritesh/venv/lib/python3.6/site-packages/mpmath/ctx_mp_python.py", line 1012, in f
return ctx.make_mpf(mpf_f(x._mpf_, prec, rounding))
File "/home/ritesh/venv/lib/python3.6/site-packages/mpmath/libmp/gammazeta.py", line 1954, in mpf_gamma
raise ValueError("gamma function pole")
ValueError: gamma function pole
I have opened this issue for this. After this is sorted, we can multivariate_gamma
will work even for negative numbers. Meanwhile, I will share my code here, for you to review.
@oscargus this PR is related to functions module. Please remove stats and add functions label. |
@ritesh99rakesh Please explain the issues which you faced and compelled you to close the old PR. |
if fuzzy_not(fuzzy_and([psym.is_integer, psym.is_positive])): | ||
raise TypeError('p must be a positive integer') | ||
from sympy import Product | ||
from sympy.abc import k |
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.
Please shift the above two imports to the top. It would be great if you can define your symbol k
under appropriate assumptions rather than importing it from sympy.abc
because it is slower.
|
||
.. [1] https://en.wikipedia.org/wiki/Multivariate_gamma_function | ||
""" | ||
psym = sympify(p) |
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.
Would it be possible to use p = sympify(p)
?
assert multivariate_gamma(n, 1).rewrite(factorial).doit() == factorial(n - 1) | ||
assert multivariate_gamma(n, 2).rewrite(factorial).doit() == sqrt(pi)*\ | ||
factorial(n - S(3)/2)*factorial(n - 1) | ||
assert multivariate_gamma(n, 3).rewrite(factorial).doit() == pi**(S(3)/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.
So many doit()
s? Please don't do it. See how, integrate
function has been defined and go according to the same lines. @sidhantnagpal What do you say?
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.
@sidhantnagpal I have replaced the >>> from sympy import symbols, conjugate, multivariate_gamma as mg
>>> x, p = symbols('x p')
>>> conjugate(mg(x, p)) # expected result
pi**((p - 1)*p/4)*Product(gamma(-_k/2 + conjugate(x) + 1/2), (_k, 1, p))
>>> conjugate(mg(x, p)) # present result
pi**((conjugate(p) - 1)*conjugate(p)/4)*Product(gamma(-_k/2 + conjugate(x) + 1/2), (_k, 1, p)) How can I explicitly impose the assumption that |
|
@sidhantnagpal I will make the suggested changes and also correct the tests. I wanted to know your view on the class implementation. What additions should I do. |
The class implementation looks OK to me, consistent with other classes for gamma functions. There could be another method |
@sidhantnagpal Please review. |
@sidhantnagpal Please review this and if this looks good to you, please consider merging. After this PR gets merged, I will be able to implement various multivariate distributions. |
@sidhantnagpal Please review this PR. |
@sidhantnagpal I have made the changes from the above suggestions. Please review. I can increase the test coverage in a new PR. |
ping @sidhantnagpal |
def _eval_is_real(self): | ||
x, p = self.args | ||
y = 2*x | ||
if isinstance(x, Symbol) or isinstance(p, Symbol): |
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.
Why restrict to symbols? It should generalise to expressions 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.
@sidhantnagpal what do you think about the following:
if x.has(Symbol) or p.has(Symbol)
If there is another better method please let me know.
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.
Is it possible to just drop these checks? (the code could be made generic to symbolic/numeric expressions)
@sidhantnagpal please review. |
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.
Looks good. Minor comments.
from sympy import Product | ||
p = Symbol('p') | ||
|
||
assert str(multigamma(x, p)) == ('pi**(p*(p - 1)/4)*Product(gamma(-_k/2 + x + 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.
You can avoid string comparison by making use of dummy_eq
(here and in other places).
@@ -137,8 +136,7 @@ def eval(cls, arg): | |||
|
|||
if arg.is_positive: | |||
return coeff*sqrt(S.Pi) / 2**n | |||
else: | |||
return 2**n*sqrt(S.Pi) / coeff | |||
return 2**n*sqrt(S.Pi) / coeff |
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.
unnecessary change
@@ -659,8 +659,7 @@ def eval(cls, n, z): | |||
if n.is_Number: | |||
if n is S.Zero: | |||
return S.Infinity | |||
else: | |||
return S.Zero | |||
return S.Zero |
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.
unnecessary change
@@ -739,24 +737,21 @@ def _eval_expand_func(self, **hints): | |||
def _eval_rewrite_as_zeta(self, n, z, **kwargs): | |||
if n >= S.One: | |||
return (-1)**(n + 1)*factorial(n)*zeta(n + 1, z) | |||
else: | |||
return self | |||
return self |
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.
unnecessary change
|
||
def _eval_as_leading_term(self, x): | ||
from sympy import Order | ||
n, z = [a.as_leading_term(x) for a in self.args] | ||
o = Order(z, x) | ||
if n == 0 and o.contains(1/x): | ||
return o.getn() * log(x) | ||
else: | ||
return self.func(n, z) | ||
return self.func(n, 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.
unnecessary change
…or .is_real in multigamma and gamma
17202af
to
d877880
Compare
@sidhantnagpal I have made the changes. Please review. |
Not all reviews are addressed. See #16843 (comment), #16843 (comment). |
Yes, there are still some open reviews. |
Codecov Report
@@ Coverage Diff @@
## master #16843 +/- ##
============================================
- Coverage 74.507% 74.5% -0.008%
============================================
Files 623 623
Lines 161379 161412 +33
Branches 37875 37882 +7
============================================
+ Hits 120240 120252 +12
- Misses 35809 35829 +20
- Partials 5330 5331 +1 |
398fcd0
to
2eeb7a7
Compare
@sidhantnagpal done. Please review. |
p = Symbol('p') | ||
_k = Dummy('_k') | ||
|
||
assert multigamma(x, p).dummy_eq(pi**(p*(p - 1)/4)*\ |
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.
Continued line markers \
are not required when enclosed in braces.
I left a comment for future PRs. This PR looks ready to me. |
References to other Issues or PRs
This is a new PR which was created because
I made a blunder with old PR: #16822
Brief description of what is fixed or changed
Multivariate Gamma function is added in
sympy/functions/special/gamma_functions.py
Need to discuss with mentors if this is needed or not. Will add test after finalization.
Other comments
Multivariate Gamma function
is a generalization ofGamma function
and is useful inmultivariate statistics appearing in the probability density function of the
Wishart
andinverse Wishart
distributions, and thematrix variate beta
distribution.Release Notes
multigamma
function insympy/functions/special/gamma_functions.py