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

improved render_model #3039

Merged
merged 5 commits into from
Mar 16, 2022
Merged

improved render_model #3039

merged 5 commits into from
Mar 16, 2022

Conversation

karm-patel
Copy link
Contributor

As discussed in #3023, I made the following changes in pyro.infer.inspect.py

  • added _pyro_post_param() method for provenance tracking for pyro.param.
  • added bool: render_param argument in render_model() to display param in graph.
  • added sample_param and param_constraint in dictionary returned by get_model_relations()
  • modified generate_graph_specification() and render_graph() to show param in graph returned by render_model()

@fritzo, Can you please suggest which type of test cases needs to be changed/added to test the above changes?

@karm-patel
Copy link
Contributor Author

karm-patel commented Mar 9, 2022

EDIT: I got the issue, I'm working on it.
.........................................................................
Hi @fritzo, I tried to study why this check is failing, But I'm unable to get an exact reason. It looks like adding provenance tracking for pyro.param conflicts with other methods. Can you please help me to get started to solve this problem?

@karm-patel
Copy link
Contributor Author

karm-patel commented Mar 10, 2022

Hi @fritzo, I am describing the error in detail. Previously, There was no provenance tracking for pyro.param hence the value of param was only tensor. In the previous commit, I have added a method to track provenance for pyro.param. So, the value of pyro.param is changed to ProvenanceTensor. Now Problem is - In module.py, some methods were expecting the value of pyro.param as tensor, so I detached provenance from the value of pyro.param using detach_provenance() method wherever needed.

I have also added the docstring for bool: render_param argument.

@karm-patel
Copy link
Contributor Author

Hi @fritzo, This PR is ready for review.

Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

Hi @karm216, thanks for your patience. Your code looks great! I have only minor comments.

Re: testing, I think since this is a visual output and we have no existing tests for get_model_relations() it is enough to simply rerun tutorial/source/intro_long.ipynb and commit the updated output. I think your improved rendering will improve that tutorial. Feel free to add a sentence or two to that tutorial describing the difference between sample and param nodes, if you think that's needed.

pyro/infer/inspect.py Outdated Show resolved Hide resolved
pyro/infer/inspect.py Show resolved Hide resolved
pyro/infer/inspect.py Outdated Show resolved Hide resolved
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@karm-patel
Copy link
Contributor Author

karm-patel commented Mar 15, 2022

Hi @fritzo , I made 3 changes

  1. Code changes suggested by you.
  2. Added render_params=True while calling render_model in 3 notebooks (wherever needed)
    1. intro_long.ipynb
    2. model_rendering.ipynb
    3. mle_map.ipynb
  3. Added explanation text regarding render_param and constraints in 2 tutorials.
    1. intro_long.ipynb
    2. model_rendering.ipynb

@fritzo
Copy link
Member

fritzo commented Mar 15, 2022

Hi @karm216 it looks like your changes include accidental running of black on notebooks. Can you please revert that part of the PR? Many of our tutorials were meticulously formatted prior to black, including pasting of data directly in notebook cells, and adding aligned line-comments. Running black creates sprawling and less legible notebooks for our readers.

@karm-patel
Copy link
Contributor Author

Hi @karm216 it looks like your changes include accidental running of black on notebooks. Can you please revert that part of the PR? Many of our tutorials were meticulously formatted prior to black, including pasting of data directly in notebook cells, and adding aligned line-comments. Running black creates sprawling and less legible notebooks for our readers.

Ohh that's my bad! @fritzo , I reverted changes by black. Can you please check now?

Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for updating the notebooks!

@karm-patel
Copy link
Contributor Author

Hi @fritzo , Some checks are failed. It looks like there is a problem in installing dependencies. Please let me know if there is a problem at my side.

@fritzo fritzo merged commit e1e4007 into pyro-ppl:dev Mar 16, 2022
@fritzo
Copy link
Member

fritzo commented Mar 16, 2022

It looks like github is buggy. First a microsoft bug broke github actions, then github allowed merging this PR without actions running 🤷

Thanks for the great contribution @karm216, I think many people will appreciate this feature!

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.

None yet

2 participants