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

Add Model.to_graphviz shortcut #6865

Merged
merged 9 commits into from
Aug 27, 2023

Conversation

juanitorduz
Copy link
Contributor

@juanitorduz juanitorduz commented Aug 17, 2023

Closes #6794


📚 Documentation preview 📚: https://pymc--6865.org.readthedocs.build/en/6865/

@juanitorduz juanitorduz marked this pull request as draft August 17, 2023 13:07
tests/test_model.py Outdated Show resolved Hide resolved
pymc/model.py Outdated Show resolved Hide resolved
pymc/model.py Outdated Show resolved Hide resolved
@ricardoV94 ricardoV94 changed the title Add graph methods to pm.Model class Add graph methods to Model class Aug 17, 2023
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #6865 (e2b40db) into main (accabdf) will decrease coverage by 0.70%.
Report is 13 commits behind head on main.
The diff coverage is 93.75%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6865      +/-   ##
==========================================
- Coverage   92.03%   91.33%   -0.70%     
==========================================
  Files          96       96              
  Lines       16369    16448      +79     
==========================================
- Hits        15065    15023      -42     
- Misses       1304     1425     +121     
Files Changed Coverage Δ
pymc/logprob/transforms.py 94.42% <90.90%> (-0.27%) ⬇️
pymc/distributions/multivariate.py 92.55% <100.00%> (+0.33%) ⬆️
pymc/model.py 90.92% <100.00%> (-0.10%) ⬇️
pymc/sampling/jax.py 98.30% <100.00%> (+0.04%) ⬆️

... and 4 files with indirect coverage changes

tests/test_model.py Outdated Show resolved Hide resolved
@juanitorduz juanitorduz marked this pull request as ready for review August 17, 2023 19:29
Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the to_graphviz prefix - not only to align with the original functions, but also to make it more discoverable with other (future) conversion methods. Thinking of to_networkx..

tests/test_model.py Outdated Show resolved Hide resolved
tests/test_model.py Outdated Show resolved Hide resolved
Co-authored-by: Michael Osthege <michael.osthege@outlook.com>
pymc/model.py Show resolved Hide resolved
@twiecki
Copy link
Member

twiecki commented Aug 20, 2023

We still need the string so that this shows up in the release notes.

@ricardoV94 ricardoV94 changed the title Add graph methods to Model class Add Model.to_graphviz shortcut Aug 21, 2023
pymc/model.py Outdated Show resolved Hide resolved
@ricardoV94
Copy link
Member

ricardoV94 commented Aug 21, 2023

We still need the string so that this shows up in the release notes.

The PR title is all that's needed. Then the label determines in which section it shows up.

@Armavica
Copy link
Member

I was using this today, I forgot how it worked and all the explanations assume that people use notebooks: would it make sense to add a return type (str) to the function, and explain somewhere that outside of a notebook, you would need to write this string to a file and call dot on it?

Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
@ricardoV94
Copy link
Member

I was using this today, I forgot how it worked and all the explanations assume that people use notebooks: would it make sense to add a return type (str) to the function, and explain somewhere that outside of a notebook, you would need to write this string to a file and call dot on it?

Do you mind opening an issue for that? I don't think it should block this issue

@michaelosthege michaelosthege merged commit ddd1d4b into pymc-devs:main Aug 27, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add graphviz method to Model
5 participants