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

[FR] Predictive with deterministic site in the guide #3358

Open
OlaRonning opened this issue Apr 19, 2024 · 6 comments
Open

[FR] Predictive with deterministic site in the guide #3358

OlaRonning opened this issue Apr 19, 2024 · 6 comments
Labels
enhancement help wanted Issues suitable for, and inviting external contributions

Comments

@OlaRonning
Copy link
Member

OlaRonning commented Apr 19, 2024

Hi,

I'm working on a project where we would like to access the output of an NN in the guide when using Predictive. We've implemented it using a deterministic site in the guide. The program boils down to the following.

import pyro
from pyro.infer import Predictive
from pyro.distributions import Normal
import torch

def model():
    pyro.deterministic('m_deter', torch.tensor(1.))
    pyro.sample('x', Normal(torch.zeros(()), torch.ones(())))

def guide():
    pyro.deterministic('g_deter', torch.tensor(1.))
    pyro.sample('x', Normal(torch.zeros(()), torch.ones(())))


Predictive(
  model=model, 
  guide=guide, 
  return_sites=('model_site', 'guide_site', 'x'), 
  num_samples=1)() # Includes m_deter but not g_deter

We would like for both m_deter and g_deter to be included. It looks like Predictive currently only considers model sites for return sites. Would it be possible to expand it so we can include deterministic sites from the guide?

@fritzo fritzo added enhancement help wanted Issues suitable for, and inviting external contributions labels Apr 20, 2024
@fritzo
Copy link
Member

fritzo commented Apr 20, 2024

This looks reasonable to me. Since deterministic guide sites are usually ignored (e.g. in AutoGuides) I think we may want to gate this new behavior by an arg like return_deterministic_guide_sites: bool or something.

@OlaRonning
Copy link
Member Author

A guard makes sense. I'll give an implementation a shot.

@SarthakNikhal
Copy link

@OlaRonning Can I help with this issue?

@OlaRonning
Copy link
Member Author

@SarthakNikhal absolutely. Feel free to look at my WIP PR. I wrote the unittest relatively fast; you can probably develop a more suitable one.

@SarthakNikhal
Copy link

@OlaRonning Okay. What can I do better? Also, what other unit tests can you think of

@OlaRonning
Copy link
Member Author

I would make the test cover four cases:

  1. return_deterministic is true and no return_sites. returned samples should include all deterministic sites in the guide.
  2. return_deterministic is true and return_sites includes one of two deterministic sites in the guide. returned samples should only include the deterministic guide site in return_sites.
  3. return_determininistic is true and there are no deterministic sites in the guide. returned samples should be the same as when return_deterministic is false.
  4. return_deterministic is false and there is a deterministic site in the guide. the returned samples should not include the deterministic site from the guide.

You'd probably want to check both that sites are included in the returned samples and that their values are as expected. I believe you can work directly on the aleatory_science branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted Issues suitable for, and inviting external contributions
Projects
None yet
Development

No branches or pull requests

3 participants