Skip to content
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

pdf method of MarginalDistribution syntactically incorrect #15859

Open
oscargus opened this issue Jan 27, 2019 · 11 comments · May be fixed by #26132
Open

pdf method of MarginalDistribution syntactically incorrect #15859

oscargus opened this issue Jan 27, 2019 · 11 comments · May be fixed by #26132
Labels
Could Close Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. stats

Comments

@oscargus
Copy link
Contributor

While cleaning up the code, I realized the following problem. However, as I am not that familiar with the stats module and therefore wasn't able to write a test that covers the potential issues, I open an issue.

Consider the method:

def pdf(self, *x):
expr, rvs = self.args[0], self.args[1]
marginalise_out = [i for i in random_symbols(expr) if i not in self.args[1]]
syms = [i.pspace.symbol for i in self.args[1]]
for i in expr.atoms(Indexed):
if isinstance(i, Indexed) and isinstance(i.base, RandomSymbol)\
and i not in rvs:
marginalise_out.append(i)
if isinstance(expr, CompoundDistribution):
syms = Dummy('x', real=True)
expr = expr.args[0].pdf(syms)
elif isinstance(expr, JointDistribution):
count = len(expr.domain.args)
x = Dummy('x', real=True, finite=True)
syms = [Indexed(x, i) for i in count]
expr = expression.pdf(syms)
return Lambda(syms, self.compute_pdf(expr, marginalise_out))(*x)

The primary issue is

expr = expression.pdf(syms)

where expression is not defined. Should probably be expr. However, looking at
expr = expr.args[0].pdf(syms)

it may be that it should instead be expr.args[0].

On

marginalise_out = [i for i in random_symbols(expr) if i not in self.args[1]]
syms = [i.pspace.symbol for i in self.args[1]]

one may replace args[1] with rvs

Now,

syms = [i.pspace.symbol for i in self.args[1]]

is redefined by
syms = Dummy('x', real=True)

and
syms = [Indexed(x, i) for i in count]

and, hence, it should probably be moved to an else: clause just before
return Lambda(syms, self.compute_pdf(expr, marginalise_out))(*x)

First, though, it would be good to make sure that this code is covered by a few tests since there are many things that are not obvious when just browsing the code. For example, the arguments to pdf are redefined on line

x = Dummy('x', real=True, finite=True)

which may be OK or just an unfortunate variable naming.

@oscargus oscargus added the stats label Jan 27, 2019
@supreet11agrawal
Copy link
Contributor

I am studying the stats module so I would like to solve this one. I might take some time though

@oscargus
Copy link
Contributor Author

Sure! Go ahead!

@Sc0rpi0n101
Copy link
Member

@oscargus Is this still an issue?
I don't see it fixed in current master

@Sc0rpi0n101
Copy link
Member

Also,
sympy/sympy/stats/joint_ry.py

Line 323

elif isinstance(expr, JointDistribution):
            count = len(expr.domain.args)
            x = Dummy('x', real=True, finite=True)
            syms = [Indexed(x, i) for i in count]
            expr = expression.pdf(syms)

We are checking if expr is an instance of JointDistribuion
and Line 327 is expr = expression.pdf(syms)
and in Line 158

    def pdf(self, *args):
        return self.density.args[1]

So, I think it will be expr which is an instance of JointDistribution and uses syms as args

@Sc0rpi0n101
Copy link
Member

as for syms, it does not look like it is doing anything else in the code,
I tried putting it in else just before line 328
Did not have any test fails.

So, I'm trying to see if new tests are required

@oscargus
Copy link
Contributor Author

oscargus commented Apr 8, 2019

I do not think that this is fixed, no.

Adding at least one test is required as the incorrect line for sure is not covered now.

@supreet11agrawal
Copy link
Contributor

Currently, I am working on this. This is not as simple as it looks. Joint Random Variables in Sympy were implemented last year so they have a lot of vulnerabilities as of now. Tests are quite redundant in terms of checking the working of the module. That is why this will take some time.

@Sc0rpi0n101
Copy link
Member

Yeah, no problem.
It was 3 months old and had no activity, so I was just asking.
If you're working on it, go ahead.

@Smit-create
Copy link
Member

I am working on it.

@Smit-create
Copy link
Member

def pdf(self, *x):
expr, rvs = self.args[0], self.args[1]
marginalise_out = [i for i in random_symbols(expr) if i not in rvs]
if isinstance(expr, CompoundDistribution):
syms = Dummy('x', real=True)
expr = expr.args[0].pdf(syms)
elif isinstance(expr, JointDistribution):
count = len(expr.domain.args)
x = Dummy('x', real=True, finite=True)
syms = tuple(Indexed(x, i) for i in count)
expr = expr.pdf(syms)
else:
syms = tuple(rv.pspace.symbol if isinstance(rv, RandomSymbol) else rv.args[0] for rv in rvs)
return Lambda(syms, self.compute_pdf(expr, marginalise_out))(*x)

Looking into the issue, I see that all the pointed erros are been resolved in current master. I think we can add a test for the same.

@Smit-create
Copy link
Member

@Upabjojr @czgdp1807 Please have a look if this can be closed.

@Smit-create Smit-create added the Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. label Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Could Close Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. stats
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants