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

Fix string representation for Bounded distributions #4216

Closed
KoljaBauer opened this issue Nov 10, 2020 · 4 comments · Fixed by #4217
Closed

Fix string representation for Bounded distributions #4216

KoljaBauer opened this issue Nov 10, 2020 · 4 comments · Fixed by #4217

Comments

@KoljaBauer
Copy link

Description of your problem

I am trying to render a pym3 model as a graph. Calling pymc3.model_to_graphviz(model) with a model that contains Bounded variables (variables with a distribution wrapped by a bounded distribution) leads to an error:

import pymc3

with pymc3.Model(name="test") as model:
        var = pymc3.Bound(pymc3.Normal, lower=1)('test_var', mu=1, sigma=1)
        img = pymc3.model_to_graphviz(model)

Please provide the full traceback.

Traceback (most recent call last):
  File "./replicate_bug.py", line 13, in <module>
    test_graphviz()
  File "./replicate_bug.py", line 9, in test_graphviz
    img = pymc3.model_to_graphviz(model)
  File "/home/new_env/lib/python3.8/site-packages/pymc3/model_graph.py", line 233, in model_to_graphviz
    return ModelGraph(model).make_graph()
  File "/home/new_env/lib/python3.8/site-packages/pymc3/model_graph.py", line 213, in make_graph
    self._make_node(var_name, graph)
  File "/home/new_env/lib/python3.8/site-packages/pymc3/model_graph.py", line 149, in _make_node
    label = str(v).replace(" ~ ", "\n~\n")
  File "/home/new_env/lib/python3.8/site-packages/pymc3/model.py", line 83, in __str__
    return self._str_repr(formatting="plain", **kwargs)
  File "/home/new_env/lib/python3.8/site-packages/pymc3/model.py", line 77, in _str_repr
    return self.distribution._str_repr(name=name, dist=dist, formatting=formatting)
  File "/home/new_env/lib/python3.8/site-packages/pymc3/distributions/distribution.py", line 176, in _str_repr
    param_values = [
  File "/home/new_env/lib/python3.8/site-packages/pymc3/distributions/distribution.py", line 177, in <listcomp>
    get_repr_for_variable(getattr(dist, x), formatting=formatting) for x in param_names
AttributeError: '_ContinuousBounded' object has no attribute 'distribution'

Graphviz was installed by executing the following steps:

git clone https://gitlab.com/graphviz/graphviz.git
cd graphviz
./autogen.sh
./configure
make
make install
cd ..
pip install graphviz

Proposed solution:

In method _str_repr of distribution.py, replace

param_values = [
            get_repr_for_variable(getattr(dist, x), formatting=formatting) for x in param_names
        ]

with

param_values = []
        for x in param_names:
            if hasattr(dist, x):
                if getattr(dist, x) is None:
                    param_values.append("")
                else:
                    param_values.append(get_repr_for_variable(getattr(dist, x), formatting=formatting))
            else:
                param_values.append("")

and additionally:
In method _distr_name_for_repr of distribution.py, add:

#If the distribution is Bounded, print the wrapped distribution
if(self.__class__.__name__ == "_ContinuousBounded"):
    return "Bound " + self._wrapped.__class__.__name__

Versions and main components

@twiecki
Copy link
Member

twiecki commented Nov 10, 2020

Thanks for the report and proposed fix @KoljaBauer, want to do a PR?

@AlexAndorra
Copy link
Contributor

As Thomas said, thanks for reporting and proposing a fix @KoljaBauer !
I'm also pinging @Spaak here, as he did a pretty in-depth PR about str representations recently

@Spaak
Copy link
Member

Spaak commented Nov 10, 2020

I third the thanks for the report and proposing a fix @KoljaBauer! :)

As for the fix, I think it's probably better to fix this at the level of _Bounded (overriding), rather than change the generic parent code in Distribution to account for this special case. (I'd be happy to fix it, or let me know if you want to have a go at it yourself @KoljaBauer).

@KoljaBauer
Copy link
Author

@Spaak Thank you for addressing it so quick!

michaelosthege pushed a commit that referenced this issue Nov 15, 2020
* handling of parameters equal to None

* improved type juggling of bounds

* updating str repr for bounded variables

* adding bounded variable to str/_repr_latex test

* updating test

* black formatting

* removing unused import

* change string to r-string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants