Skip to content

Do not expect model variables to always have str_repr #6311

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

Closed
ricardoV94 opened this issue Nov 18, 2022 · 5 comments · Fixed by #6942
Closed

Do not expect model variables to always have str_repr #6311

ricardoV94 opened this issue Nov 18, 2022 · 5 comments · Fixed by #6942
Labels

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Nov 18, 2022

When we create RVs via the the Distribution interface, they get some str_repr methods glued to them:

# add in pretty-printing support
rv_out.str_repr = types.MethodType(str_for_dist, rv_out)
rv_out._repr_latex_ = types.MethodType(
functools.partial(str_for_dist, formatting="latex"), rv_out
)

The Model.str_repr then expects all named variables to have this method, which fails for automatically generated RVs such as those in automatic imputation

import numpy as np
import pymc as pm

with pm.Model() as m:
   pm.Normal("x", observed=[np.nan, 0, 0])
m.str_repr()  # Called automatically in a Jupyter environment for pretty-printing
AttributeError: 'TensorVariable' object has no attribute 'str_repr'

It also which makes it annoying to replace / register variables manually:

x = pm.Normal.dist()  # No `str_repr` attached
with pm.Model() as m:
  m.register_rv(x, name="x")
m.str_repr()  # Same error

Suggestion

Do not rely on the methods being attached for model.str_repr

rv_reprs = [rv.str_repr(formatting=formatting, include_params=include_params) for rv in all_rv]

Instead just use a dispatch base approach to pretty-print any model variables based on their Op types as discussed elsewhere.

@ricardoV94
Copy link
Member Author

CC @larryshamalama in case you want to take on this one

@ricardoV94 ricardoV94 changed the title Do not attach / expect RVs to have str_repr Do not expect model variables to always have str_repr Nov 18, 2022
@larryshamalama
Copy link
Member

I'd love to! However, I'm quite busy this month and probably can only get to this once the semester is over. If anyone else reading this thread wants to take this issue in the meanwhile, you are more than welcome.

Adding @ricardoV94's relevant comment from a discussion thread about this.

@larryshamalama
Copy link
Member

Some progress is being made, but mostly taking nearly verbatim the code from aeppl/printing.py...

Should we still attach str_repr to pm.Model instances and RVs created via the Distribution interface? It is my understanding that Latex printing in a Jupyter notebook relies on RV/model instances having a _repr_latex_ method. In other words,

import pymc as pm
from pymc.printing import str_repr # newly created dispatch function

with pm.Model() as model:
    x = pm.Normal("x", 0., 1.)
    y = pm.Normal.dist(0., 1.)
    model.register(y, name="y")

# (1)
x.str_repr()
y.str_repr()
model.str_repr()

# (2)
str_repr(x)
str_repr(y)
str_repr(model)

Should we expect (1), (2) or both of the examples above to work?

@ricardoV94
Copy link
Member Author

I think it's enough if the Model has the pretty rendering, we don't need each variable to have it by itself.

Would be good to adapt the code to work with dispatching so that RVs/Ops created by third party libraries can be given nice printing as well without PyMC having to know about them.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Feb 22, 2023

I think we can keep attaching it to the Model and variables created via Distributions, we should just make sure the Model does not expect all it's variables to have the attached method (it can call the function itself).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants