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

Render deterministic functions #3266

Merged
merged 7 commits into from
Sep 20, 2023
Merged

Render deterministic functions #3266

merged 7 commits into from
Sep 20, 2023

Conversation

r3v1
Copy link
Contributor

@r3v1 r3v1 commented Sep 14, 2023

Addresses #3134 and pull request #3259.

Copy link
Member

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

@r3v1 thanks for doing this! I left some comments requesting changes below. Can you also add the unit tests from #3259?

pyro/infer/inspect.py Outdated Show resolved Hide resolved
pyro/infer/inspect.py Outdated Show resolved Hide resolved
pyro/poutine/trace_messenger.py Outdated Show resolved Hide resolved
Comment on lines 536 to 538
rv_label = rv.replace(
"$params", ""
) # incase of neural network parameters
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert this change, since it's independent of the main goal of this PR? You're welcome to open a separate issue or PR with suggested changes to rendering settings like this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but as I define rv_label in the if clause, it should also be defined in the else clause. Otherwise, it will be undefined

Copy link
Member

Choose a reason for hiding this comment

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

You can just revert to using rv, once you revert this chance there's no need for rv_label.

Copy link
Member

Choose a reason for hiding this comment

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

@r3v1 looks like this change did not get reverted

pyro/infer/inspect.py Outdated Show resolved Hide resolved
pyro/infer/inspect.py Outdated Show resolved Hide resolved
pyro/infer/inspect.py Outdated Show resolved Hide resolved
@eb8680 eb8680 linked an issue Sep 14, 2023 that may be closed by this pull request
@eb8680
Copy link
Member

eb8680 commented Sep 14, 2023

@r3v1 can you also run make format and fix any remaining errors raised by make lint to appease the linter and make CI run unit tests?

@r3v1 r3v1 requested a review from eb8680 September 17, 2023 17:21
Copy link
Member

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

@r3v1 can you add a unit test like test_get_model_relations from #3259 that exercises your new functionality, and can you also add a quick example at the end of the model rendering tutorial to demonstrate how to use it?

For the example, you can just copy the model that's already defined in the "Rendering the parameters" section and add a couple of new deterministic variables, e.g.:

def model_deterministic(data):
    sigma = pyro.param("sigma", torch.tensor([1.]), constraint=constraints.positive)
    mu = pyro.param("mu", torch.tensor([0.]))
    x = pyro.sample("x", dist.Normal(mu, sigma))
    log_y = pyro.sample("y", dist.Normal(x, 1))
    y = pyro.deterministic("y", log_y.exp())
    with pyro.plate("N", len(data)):
        eps_z_loc = pyro.sample("eps_z_loc", dist.Normal(0, 1))
        z_loc = pyro.deterministic("z_loc", eps_z_loc + x, event_dim=0)
        pyro.sample("z", dist.Normal(z_loc, y), obs=data)

data = torch.ones(10)
pyro.render_model(
    model_deterministic,
    model_args=(data,),
    render_params=True,
    render_distributions=True,
    render_deterministic=True
)

Comment on lines 536 to 538
rv_label = rv.replace(
"$params", ""
) # incase of neural network parameters
Copy link
Member

Choose a reason for hiding this comment

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

@r3v1 looks like this change did not get reverted

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

@r3v1
Copy link
Contributor Author

r3v1 commented Sep 18, 2023

As suggested earlier, rv_label = rv.replace("$params", "") is necessary to correctly render the graphs:

Without it:
model

And with it:
model

Do I push the change?

Copy link
Member

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

Do I push the change?

Sorry, I think there was a problem with the diff I was looking at on GitHub. I was specifically requesting the removal of the .replace("$params", "") method call, which I now see you actually did already. The most recent commit as of this review produces the second of your two figures, right?

Other than a couple minor comments I left below, I think this is good to go as a first PR assuming tests pass since it retains the current dev behavior of get_dependencies and render_model as the default. We can change that behavior in a followup PR.

pyro/infer/inspect.py Outdated Show resolved Hide resolved
pyro/infer/inspect.py Show resolved Hide resolved
pyro/test.py Outdated Show resolved Hide resolved
@r3v1
Copy link
Contributor Author

r3v1 commented Sep 19, 2023

Do I push the change?

Sorry, I think there was a problem with the diff I was looking at on GitHub. I was specifically requesting the removal of the .replace("$params", "") method call, which I now see you actually did already. The most recent commit as of this review produces the second of your two figures, right?

The most recent commit 6fbb58c produces the first picture, so I'm pushing the fix along the other suggestions in the next commit.

Copy link
Member

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

LGTM, outstanding issues can be addressed in followup PRs. Thanks again for taking this on!

@eb8680 eb8680 merged commit 99633ae into pyro-ppl:dev Sep 20, 2023
9 checks passed
@r3v1
Copy link
Contributor Author

r3v1 commented Nov 13, 2023

Hi @eb8680, do you plan to make a new release with this change?

@eb8680
Copy link
Member

eb8680 commented Nov 13, 2023

@r3v1 yes, we'll do a minor release sometime soon. In the meantime you can follow the instructions from the README to install the dev branch, where this PR landed: https://github.com/pyro-ppl/pyro#installing-pyro-dev-branch

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.

Render deterministic functions of random variables
2 participants