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

Remove MultiObservedRV in v4 #4534

Closed
brandonwillard opened this issue Mar 13, 2021 · 3 comments
Closed

Remove MultiObservedRV in v4 #4534

brandonwillard opened this issue Mar 13, 2021 · 3 comments

Comments

@brandonwillard
Copy link
Contributor

brandonwillard commented Mar 13, 2021

MultiObservedRV support hasn't been implemented in v4 (because I couldn't really tell what it was). @OriolAbril explained it to me here, and it sounds like something we should remove entirely.

I'm opening this issue so that someone can inform us of any important functionalities that MultiObservedRV exclusively provides, so that we can implement those in some other way before removing the old MultiObservedRV-related code.

@ricardoV94
Copy link
Member

This is somewhat related to the DensityDist, no? We should also decide what to do with that.

@lucianopaz
Copy link
Contributor

DensityDist was the only distribution that used MultiObservedRV. It was a hackish way to pass distribution parameters into the user's supplied logp.

If you had a logp function that was defined like this:

def logp(value, mu, sigma):
    return -0.5*np.log(2 * np.pi) - at.log(sigma) - 0.5*((value - mu)/sigma)**2

In v3 you had to do this to be able to pass parameter values to value, mu and sigma

with pm.Model():
    mu = pm.Normal("mu")
    sigma = pm.HalfNormal("sigma")
    pm.DensityDist("dist", logp, observed={"value": np.ones(10), "mu": mu, "sigma": sigma})

In #5026, we completely changed the API of DensityDists and made it much more similar to other distributions. With the new API, you need to do:

with pm.Model():
    mu = pm.Normal("mu")
    sigma = pm.HalfNormal("sigma")
    pm.DensityDist("dist", mu, sigma, logp=logp, observed=np.ones(10))

The new DensityDist relies on the positional arguments to define the distribution's parameters, and those are then supplied into all the customizable methods: logp, logcdf, random and get_moment.

If #5026 is approved and merged, we should just completely remove MultiObservedRV.

@ricardoV94
Copy link
Member

This Class seems to not be present anymore in the codebase, so the only thing left is to remove old tests:

@pytest.mark.xfail(reason="MultiObservedRV is no longer used in v4")
def test_multiple_observed_rv_without_observations(self):
with pm.Model():
mu = pm.Normal("mu")
x = pm.DensityDist( # pylint: disable=unused-variable
"x", mu, logp=lambda value, mu: pm.Normal.logp(value, mu, 1), observed=0.1
)
inference_data = pm.sample(100, chains=2, return_inferencedata=True)
test_dict = {
"posterior": ["mu"],
"sample_stats": ["lp"],
"log_likelihood": ["x"],
"observed_data": ["value", "~x"],
}
fails = check_multiple_attrs(test_dict, inference_data)
assert not fails
assert inference_data.observed_data.value.dtype.kind == "f"
@pytest.mark.xfail(reason="MultiObservedRV is no longer used in v4")
@pytest.mark.parametrize("multiobs", (True, False))
def test_multiobservedrv_to_observed_data(self, multiobs):
# fake regression data, with weights (W)
np.random.seed(2019)
N = 100
X = np.random.uniform(size=N)
W = 1 + np.random.poisson(size=N)
a, b = 5, 17
Y = a + np.random.normal(b * X)
with pm.Model():
a = pm.Normal("a", 0, 10)
b = pm.Normal("b", 0, 10)
mu = a + b * X
sigma = pm.HalfNormal("sigma", 1)
w = W
def weighted_normal(value, mu, sigma, w):
return w * pm.Normal.logp(value, mu, sigma)
y_logp = pm.DensityDist( # pylint: disable=unused-variable
"y_logp", mu, sigma, w, logp=weighted_normal, observed=Y, size=N
)
idata = pm.sample(
20, tune=20, return_inferencedata=True, idata_kwargs={"density_dist_obs": multiobs}
)
multiobs_str = "" if multiobs else "~"
test_dict = {
"posterior": ["a", "b", "sigma"],
"sample_stats": ["lp"],
"log_likelihood": ["y_logp"],
f"{multiobs_str}observed_data": ["y", "w"],
}
fails = check_multiple_attrs(test_dict, idata)
assert not fails
if multiobs:
assert idata.observed_data.y.dtype.kind == "f"

ricardoV94 added a commit to ricardoV94/pymc that referenced this issue Jan 13, 2022
ricardoV94 added a commit to ricardoV94/pymc that referenced this issue Jan 13, 2022
ricardoV94 added a commit to ricardoV94/pymc that referenced this issue Jan 13, 2022
ricardoV94 added a commit to ricardoV94/pymc that referenced this issue Jan 13, 2022
ricardoV94 added a commit to ricardoV94/pymc that referenced this issue Jan 13, 2022
ricardoV94 added a commit to ricardoV94/pymc that referenced this issue Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants