pymc icon indicating copy to clipboard operation
pymc copied to clipboard

ZeroInflatedPoisson and ZeroInflatedNegativeBinomial show up as Deterministic on graphviz

Open twiecki opened this issue 3 years ago • 2 comments

It's probably because they are not pure RandomVariables. We need to update the model_graph logic to accommodate them

twiecki avatar May 15 '22 14:05 twiecki

The NormalMixture RV has the same issue.

with pm.Model() as mixture_model:
    weights = pm.Dirichlet('w', np.ones(2))
    sigma2 = pm.InverseGamma('sigma2', shape=2, alpha=alpha_0, beta=beta_0)
    mu = pm.Normal('mu', shape=2, mu=mu_0, sigma=np.sqrt(sigma2 / nu_0))
    pm.NormalMixture('x', w=weights, mu=mu, sigma=np.sqrt(sigma2), observed=data)

pm.model_to_graphviz(mixture_model)
image

cscheffler avatar Jul 22 '22 09:07 cscheffler

Any non pure RV shows up as a Deterministic. We need to update the logic to check if a variable is in model.basic_RVs instead of checking if it's a RandomVariable which not all model variables are since we introduced SymbolicDistributions

ricardoV94 avatar Jul 22 '22 13:07 ricardoV94

@ricardoV94 bringing the discussion back here.

Short summary: While attempting to refactor graphviz support for SymbolicRVs in #6149, we realized that it would be good to use the distribution's name as input rather than what we have available in _print_name for Aesara random variables. This is because SymbolicRVs use distributions as input (e.g. TruncatedRV, CensoredRV, MixtureRV, etc.); therefore, PyMC-like random variable classes were introduce to provide descriptive _print_names in #6219. However, a dispatching approach to graphviz display would be better in the longer run, hence reverting the previously mentioned PR in #6249.

https://github.com/pymc-devs/pymc/blob/2dc49e5502edcfa98df18247e299aba12f8448e5/pymc/model_graph.py#L157-L178

Should we dispatch the creation of kwargs for graphviz nodes? Would it be better to create classmethods within distribution classes or dispatch functions outside of them (akin to dispatching of moments and logps)

larryshamalama avatar Oct 31 '22 06:10 larryshamalama

The part where we define symbol should be done with dispatching, the rest can stay the same. This is what will allow flexibility for SymbolicRVs to provide fancy names based on their inputs. Vanilla RandomVariables will always return just Normal or whatever, independent of their parameters.

It should be done with dispatching following the approach taken in the rest of the library

ricardoV94 avatar Nov 03 '22 08:11 ricardoV94

The issue of making it a clever name is separate from showing nodes as RVs instead of Deterministic though, we should open a separate issue for that if it doesn't exist already

ricardoV94 avatar Nov 03 '22 09:11 ricardoV94