-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Multinomial coefficients #18933
Comments
Sir, I would like to work on this issue. I will be sending a PR soon. |
Yes, but I'd expect it to be presented as a |
Sure sir. |
Except for integers, where all should multinomial be applicable? |
In the wolfram function repository, I see they are using a generalized definition using gamma functions. So it can be defined all over the complexes and differentiable. And I already see that factorial or binomial in sympy are using the definition from gamma, so the definition from wolfram can be used. |
http://functions.wolfram.com/GammaBetaErf/Multinomial/02/ |
Yes |
This comment has been minimized.
This comment has been minimized.
@sylee957 would it be preferred to write the Multinomial from scratch or just implement it as a product of binomials ? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
In my opinion, I'd just implement it from scratch because expanding a multinomial into a product of binomials may not have a unique representation. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Please try to avoid off topic discussions. Some comments here were not relevant to the issue and hence are marked hidden. Even this comment is not going to solve the issue in any way but it has to be made to clarify things. |
@sylee957 what about printing multinomials ... like print_binomial in latex.py and mathml.py ? |
And binomial coefficients are multinomial coefficients. So, if they both are represented by classes then former should inherit the later. |
That's what I asked @sylee957 ... but he was talking about writing one from scratch ...
|
Okey 👍 |
Though I am not sure which design scheme should be followed. I think that, having general classes subclassed in specialised classes help in getting things done with little code. Like, a truck is a vehicle so we should go for vehicle being subclassed in truck. Though if there is any other compelling reason to do the reverse then that should be presented here. |
When inheriting the class, the args structure must be coherent or other issues can arise. I agree that multinomial without n looks better. However, it can make more difficult to keep the consistent structure with classes, so they should not inherit each other. But I’d rather have other ideas like Binomial automatically evaluate to multinomial and keep binomial a stub class for notational purpose, if it should be |
I see, |
So can we use the binomial class then? |
Can you please elaborate more on your question? |
Could we implement the multinomial coefficient as a function or it is necessary to construct a separate class for it? |
I think that the APIs for |
Sure sir. |
Just for clarification, by similarity I mean that functions provided by |
I think it is okay for subclasses to have different args. The important thing is that code should access properties rather than accessing the args directly. Then a subclass that changes the args structure just needs to override the properties that access the args. |
@czgdp1807 @oscarbenjamin Could you please go through my PR? |
I've been working on this ... is this close to what you were expecting @sylee957 @czgdp1807 @oscarbenjamin |
IMO, it looks fine to me. Can you please make a PR to show whatever you have written for |
IMO, it looks fine. Can you please make a PR to show whatever you have written for |
I'll submit a pr ... in 6-7 hrs or so. I'm soo sleepy , btw I am yet to add support for expanding the expression and for .eval , the rest are almost done @czgdp1807 |
The framework I'd begin with is from sympy import *
from sympy.core.sympify import _sympify
from sympy.functions.combinatorial.factorials import CombinatorialFunction
class multinomial(CombinatorialFunction):
@classmethod
def eval(cls, *k):
k = [_sympify(x) for x in k]
n = Add(*k)
if all(x.is_Integer for x in k):
n = Add(*k)
numer = factorial(n)
denom = Mul(*(factorial(x) for x in k))
return numer/denom
def _eval_rewrite_as_factorial(self, *args, **kwargs):
n = Add(*args)
numer = factorial(n)
denom = Mul(*(factorial(x) for x in args))
return numer/denom
def _eval_rewrite_as_binomial(self, *args, **kwargs):
pass
def _eval_rewrite_as_gamma(self, *args, **kwargs):
pass Actually, when following the sympy |
@sylee957 I wrote something similar to that ... but It was just using gamma |
For integers only, it’s better to use factorials |
Yes! |
@sylee957 @czgdp1807 I've drafted a PR ... lmk what you think. |
Yeahh. I've removed test cases where n is negative. I'll add a check too. |
@sylee957 Okay sir. |
@sylee957 @czgdp1807 Sir, I believe my version of the PR is slightly better. Also, I have squashed the commits for easy readability. Due to the squash, checks are been performed all over again. I request you to wait for some time. |
@sylee957 @czgdp1807 Please review and merge PR #18952 |
#18906 had code to calculate a given multinomial coefficient but it is stalled. It would be nice to have the ability to calculate a concrete coefficient. The following is an efficient version that could be used: def multinomial_coefficient(*e):
"""Return the multinomial coefficient (e1 + e2 + ...)! / e1! / e2! / ..."""
# https://stackoverflow.com/a/56856279/1089161
if not e: # no parameters
return 1
ibig = e.index(max(e))
num = e[ibig] + 1
rv = 1
for n in e[:ibig] + e[ibig + 1:]:
for den in range(1, n + 1):
rv *= num // den
num += 1
return rv Note that currently one can calculate all coefficients with |
I think that multinomial coefficient can be added
It won't be difficult, but the conflicting notations between mathematica and maple is making it confusing.
Some people prefer to use
while some other people prefer to use
assuming that n is loosely the summation of each k.
So it should be chosen that which kind if definition is the best
And if the second one is chosen, I think that I should rather make it like
mechanically, so it won't really be much convenient than choosing the first one. But this can be consistent with the definition of binomial.
Some references:
The text was updated successfully, but these errors were encountered: