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

Eliminate need for CompartmentalModel.transition_bwd() #2514

Merged
merged 6 commits into from
Jun 4, 2020

Conversation

fritzo
Copy link
Member

@fritzo fritzo commented May 31, 2020

Addresses #2426

This follows ideas from #2513 to replace the .transition_bwd() method with a conditioned call of the .transition_fwd() method, significantly reducing user effort to create a new model. The additional bit of logic to reverse the flow computation is implemented in a new .compute_flows() method that by default assumes a linear flow terminating in "R", but can be overridden by users to perform arbitrary computations. Only one such override is needed by our current examples.

The new .transition() method is now called under more interpretations, so various logic needs to be generalized:

  • t < self.duration now also needs to check for slice instances
  • We now need to use ExtendedBinomial everywhere, not only in .transition_bwd()
  • The SMC wrapper now needs to provide approximate variables like I_approx.

Tested

  • refactoring covered by existing tests
  • manually ran a biggish example and verified sane posterior

def transition_bwd(self, params, prev, curr, t):
R0, tau, rho = params

def compute_flows(self, prev, curr, t):
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the one model where we need a custom .compute_flows() method.

@fritzo fritzo changed the title Eliminate need for CompartmentalModel.transition_bwd() method Eliminate need for CompartmentalModel.transition_bwd() May 31, 2020
@fritzo fritzo mentioned this pull request Jun 3, 2020
2 tasks
martinjankowiak
martinjankowiak previously approved these changes Jun 4, 2020
Copy link
Collaborator

@martinjankowiak martinjankowiak left a comment

Choose a reason for hiding this comment

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

great simplification!

pyro/contrib/epidemiology/compartmental.py Outdated Show resolved Hide resolved
pyro/contrib/epidemiology/compartmental.py Outdated Show resolved Hide resolved
pyro/contrib/epidemiology/models.py Outdated Show resolved Hide resolved
@martinjankowiak martinjankowiak merged commit 8612d72 into dev Jun 4, 2020
@fritzo fritzo deleted the sir-simplify-transition branch June 5, 2020 15:30
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.

2 participants