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

Refactoring sample from dicts to named arguments + **kwargs #63

Conversation

gvegayon
Copy link
Member

@gvegayon gvegayon commented Apr 3, 2024

This PR includes the following:

  • Updates the metaclasses RandomVariable and Model to use **kwargs for sample().
  • Updates all instances of these under the submodules.
  • Within the model module, it ensures that arguments are via keywords instead of dictionaries.
  • Updates the tests to reflect the change.
  • Updates the demos under docs to reflect the change.

For a new discussion: Now that I've done this refactoring, the need to standardize the submodules became apparent.

  • When specifying the observation process for any of the models, it makes sense to have the RandomVariable.sample() method of observed class feature mean and obs. This way the HospitalizationModel would always expect the observation process to receive mean for the latent hospitalizations and obs for the observed hospitalizations.
  • Other arguments could still be passed as now **kwargs are unpacked whenever calling a RandomVariable.sample() method.
  • Like the observed, process could also have defaults for RandomVariable.sample(), e.g., n_timesteps.

@gvegayon gvegayon linked an issue Apr 3, 2024 that may be closed by this pull request
7 tasks
@gvegayon gvegayon marked this pull request as ready for review April 3, 2024 21:22
@AFg6K7h4fhy2
Copy link
Collaborator

AFg6K7h4fhy2 commented Apr 4, 2024

Want me to review this? So 47 is not bottlenecked?

Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

Some tweaks.

RE: Observation classes: as discussed in person, we should standardize the name of the predicted value and observed value keyword arguments to an Observation's sample() method. I think obs for observed (to be consistent with numpyro.sample(). For the prediction, how about pred, which is nicely consistent with obs?

GIven this, I agree that more templating makes sense. Time for an abstract Observation(RandomVariable) metaclass?

@gvegayon
Copy link
Member Author

gvegayon commented Apr 4, 2024

@AFg6K7h4fhy2 and @dylanhmorris, all done with your requests:

  • Improved doc on **kwargs: I still left a couple where it says Ignored, but that's because it is literally that.
  • Renamed mean to predicted for the observation processes.
  • Replaced a default value RandomVariable to none to avoid mutable defaults (but left a few strings, as strings are inmutable).

Copy link
Collaborator

@AFg6K7h4fhy2 AFg6K7h4fhy2 left a comment

Choose a reason for hiding this comment

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

Approve conditional on some minor comments being addressed.

@gvegayon gvegayon dismissed dylanhmorris’s stale review April 4, 2024 19:43

No big changes since last check. Most unaddressed issues will be resolved with existing issues+PRs.

@gvegayon gvegayon merged commit d12edb1 into main Apr 4, 2024
5 checks passed
@gvegayon gvegayon deleted the gvegayon/dev/60-refactor-sample-functions-to-use-named-arguments-and-kwargs branch April 4, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor sample functions to use named arguments and **kwargs
3 participants