CDCgov / PyRenew

Python package for multi-signal Bayesian renewal modeling with JAX and NumPyro.
https://cdcgov.github.io/PyRenew/
Apache License 2.0
14 stars 2 forks source link

Tutorials should demonstrate writing a `Model.model()` method #276

Open dylanhmorris opened 2 months ago

dylanhmorris commented 2 months ago

We may wish to ship a view particular Model subclasses that do common things, but in light of the pyrenew principles, I think we want to encourage users to write their own subclasses of Model and provide tutorials for that. This makes the process of understanding what goes into a model and how it all fits together less opaque.

EDIT for clarity

AFg6K7h4fhy2 commented 2 months ago

If I understand correctly what you've written, I've attempted to take this step of Model subclassing in cfaepim, as opposed to using functions for each component of the model.

This also makes model composition less opaque

Agreed.

sbidari commented 2 months ago

What is model composition referring to here? I am particularly confused as the pyrenew principles says composing models is discouraged.

dylanhmorris commented 2 months ago

Apologies, I was unclear. I meant "what goes into a model and how it all fits together". In fact, it's related to why we discourage "composing models" in the principles (i.e. we discourage having Model subclasses that contain other Model subclasses as sub-components): too much of that makes it less clear how everything fits together.

damonbayer commented 2 months ago

I'm also confused about if this issue is about writing subclasses or methods.

dylanhmorris commented 2 months ago

As things are construct ATM: a user's easiest way of designing a model would be to inherit from metaclass.Model and implement a concrete .model() method (related: I don't think we should have Model.model as a mere alias, but as a true replacement (i.o.w. I would suggest reopening #225).

If we think that's not the right pattern to encourage (versus just writing your own model() function and attaching it to an object that provides validation and inference methods, I'm open to that as an alternative or additional pattern to support.

sbidari commented 2 months ago

Thanks for the clarification @dylanhmorris

(i.e. we discourage having Model subclasses that contain other Model subclasses as sub-components)

But doesn't HospitalAdmissionsModel model class use RtInfectionsRenewalModel as a sub-component, is this discouraged under the principles?

dylanhmorris commented 2 months ago

Yes, there's a plan to refactor and change that.

sbidari commented 2 months ago

Got it, thanks @dylanhmorris

dylanhmorris commented 2 months ago

See also #227