Repository: pymc-devs/pymc
Pull request: fix default prior variable names
Fix #3588. I have also noted that `sample_prior_predictive` samples both the prior and the prior_predictive. Considering how default var names are defined now. It would be quite easy to divide the function in 2, a `sample_prior` and a `sample_prior_predictive` but it would be quite a breaking change. Are you interested in dividing the function at all? Would you rather do in now or in some future PR after having added FutureWarnings here?

I think the best idea will probably be to make this PR only about fixing #3588 so it can be merged before the discussion about prior and prior predictive. I can send another PR afterwards with whatever is decided.

Problem statement:
sample_prior_predictive returns pm.Data variables too
`sample_prior_predictive` returns values of data variables defined as `pm.Data`. I would expect it to contain only prior samples (mu, tau, theta) and prior predictive samples (obs) but not sigma. Example using 8-schools model below:

```python
import pymc3 as pm
draws = 500
chains = 2

eight_school_data = {
    'J': 8,
    'y': np.array([28., 8., -3., 7., -1., 1., 18., 12.]),
    'sigma': np.array([15., 10., 16., 11., 9., 11., 10., 18.])
}

with pm.Model() as model:
    sigma = pm.Data("sigma", eight_school_data["sigma"])
    mu = pm.Normal('mu', mu=0, sd=5)
    tau = pm.HalfCauchy('tau', beta=5)
    theta = pm.Normal('theta', mu=mu, sd=tau, shape=eight_school_data['J'])
    pm.Normal('obs', mu=theta, sd=sigma, observed=eight_school_data['y'])
    
    trace = pm.sample(draws, chains=chains)
    prior = pm.sample_prior_predictive()
print(prior.keys())
# dict_keys(['obs', 'mu', 'sigma', 'tau', 'tau_log__', 'theta'])
```

## Versions and main components

* PyMC3 Version: dev
* Theano Version: 1.0.4
* Python Version: 3.6
* Operating system: Ubuntu 18 LTS
* How did you install PyMC3: (conda/pip) pip

Hints from issue discussion:

Commit message: update docs and add test on pm.Data not being in prior

Patch to review:
diff --git a/pymc3/sampling.py b/pymc3/sampling.py
index 8374cb07da..7084cf838a 100644
--- a/pymc3/sampling.py
+++ b/pymc3/sampling.py
@@ -1292,7 +1292,7 @@ def sample_prior_predictive(samples=500,
         samples.  *DEPRECATED* - Use ``var_names`` argument instead.
     var_names : Iterable[str]
         A list of names of variables for which to compute the posterior predictive
-        samples. Defaults to ``model.named_vars``.
+        samples. Defaults to both observed and unobserved RVs.
     random_seed : int
         Seed for the random number generator.
 
@@ -1305,8 +1305,10 @@ def sample_prior_predictive(samples=500,
     model = modelcontext(model)
 
     if vars is None and var_names is None:
-        vars = set(model.named_vars.keys())
-        vars_ = model.named_vars
+        prior_pred_vars = model.observed_RVs
+        prior_vars = get_default_varnames(model.unobserved_RVs, include_transformed=True)
+        vars_ = [var.name for var in prior_vars + prior_pred_vars]
+        vars = set(vars_)
     elif vars is None:
         vars = var_names
         vars_ = vars
diff --git a/pymc3/tests/test_sampling.py b/pymc3/tests/test_sampling.py
index 9fd36af11d..db431ade4f 100644
--- a/pymc3/tests/test_sampling.py
+++ b/pymc3/tests/test_sampling.py
@@ -505,12 +505,14 @@ def test_ignores_observed(self):
         observed = np.random.normal(10, 1, size=200)
         with pm.Model():
             # Use a prior that's way off to show we're ignoring the observed variables
+            observed_data = pm.Data("observed_data", observed)
             mu = pm.Normal("mu", mu=-100, sigma=1)
             positive_mu = pm.Deterministic("positive_mu", np.abs(mu))
             z = -1 - positive_mu
-            pm.Normal("x_obs", mu=z, sigma=1, observed=observed)
+            pm.Normal("x_obs", mu=z, sigma=1, observed=observed_data)
             prior = pm.sample_prior_predictive()
 
+        assert "observed_data" not in prior
         assert (prior["mu"] < 90).all()
         assert (prior["positive_mu"] > 90).all()
         assert (prior["x_obs"] < 90).all()


Code review comments:
diff_hunk: @@ -1305,8 +1305,10 @@ def sample_prior_predictive(samples=500,
     model = modelcontext(model)
 
     if vars is None and var_names is None:
-        vars = set(model.named_vars.keys())
-        vars_ = model.named_vars
+        prior_pred_vars = model.observed_RVs
+        prior_vars = get_default_varnames(model.unobserved_RVs, include_transformed=True)
path: pymc3/sampling.py
text: @user1:
This will exclude potentials (model.potentials)

@author:
I did not know these existed. Should they be included in `prior_vars` or `prior_pred_vars`?

@user1:
Potentials are a way to add arbitrary terms to the computation of the logp. You can use them to add constraints to the model. I will add them to `prior_vars`