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

Predictive and Deterministic tutorial #2852

Merged
merged 2 commits into from
Jun 17, 2021

Conversation

mtsokol
Copy link
Contributor

@mtsokol mtsokol commented May 30, 2021

Hi @fritzo!

Sorry for a delay, here's a PR with a Predictive and deterministic tutorial discussed in #2803.

As proposed it's extended with a SVI method (with user defined guide and AutoGuide).

As you suggested, in the next PR I will add "Advanced Usage" section with poutine routines instead of Predictive class for further comparison.

Here are links to the notebook from the PR:

https://gist.github.com/mtsokol/fe00f78bb0b925f986d241622c2ec019
https://nbviewer.jupyter.org/gist/mtsokol/fe00f78bb0b925f986d241622c2ec019

What should I improve/modify?

My questions:

  1. When performing inference with SVI it was failing with:
ValueError: Error while computing log_prob at site 'counts':
The value argument must be within the support

Although validate_args=False in GammaPoisson fixed it, actual values, which I was inspecting in a debugger, seemed within GammaPoissons support.

  1. The guide I defined is rather arbitrary. Is it OK to use Delta this way or is it wrong?

Thank you for your help!

view jupyter notebook

@fritzo
Copy link
Member

fritzo commented Jun 14, 2021

Hi @mtsokol this looks great, thanks for creating a tutorial! (and sorry for my delay in reviewing.) I have only minor comments:

  • I didn't know about %config InlineBackend.figure_format='retina'. If this ends up looking good in your tutorial, we might want to change all other tutorials as well. (but let's check, since IIRC manually setting dpi=300 or svg output broke some tutorials in the past; we'll see on docs.pyro.ai once this merges).
  • I think batch_size -> batch_shape and event_size -> event_shape would be more standard names.
  • The model looks great, I like the use of sklearn's make_regression helper.
  • Is there a reason you're using validate_args=False in the model? I'd rather encourage new users to use validation whenever possible. If you're simply trying to speed up MCMC, you might instead set jit_compile=True.
  • nit: I think it would help to label both x and y axes on all figures. In particular, it took me a while to figure out what the "rate mean" and "rate std" plots show.
  • Wherever you print(predictive.keys()) I think it would be helpful to additionally print sizes, say:
    for k, v in predictive.items():
        print(f"{k}: {tuple(v.shape)}")

@mtsokol
Copy link
Contributor Author

mtsokol commented Jun 16, 2021

@fritzo Thanks for a review!

Is there a reason you're using validate_args=False in the model?

Without validate_args=False SVI is failing with:

ValueError: Error while computing log_prob at site 'counts':
The value argument must be within the support

Here's a notebook with a full reproduction:
https://colab.research.google.com/drive/165osM9-jI2C8UbCppHTQe0ZpTc-S9WL-?usp=sharing

Is it a bug or my mistake in SVI part?

@fritzo
Copy link
Member

fritzo commented Jun 16, 2021

Hmm, are your counts y_ actually integers? What if you

  y_ = torch.tensor((y**3)/100000. + 10., dtype=torch.float)
+ y_.round_().clamp_(min=0)

@mtsokol
Copy link
Contributor Author

mtsokol commented Jun 16, 2021

@fritzo Yes, it was my mistake. Now all working! I pushed changes with all fixes from the review.

Also gist with a current version:
https://gist.github.com/mtsokol/39f6d679834432bf29e1c35e79f547cf

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 creating this tutorial!

@fritzo fritzo merged commit 9b5cd3d into pyro-ppl:dev Jun 17, 2021
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