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

Consider renaming DelayObservations and family #80

Closed
seabbs opened this issue Feb 27, 2024 · 9 comments · Fixed by #162
Closed

Consider renaming DelayObservations and family #80

seabbs opened this issue Feb 27, 2024 · 9 comments · Fixed by #162
Assignees
Labels

Comments

@seabbs
Copy link
Collaborator

seabbs commented Feb 27, 2024

As the title. This is currently hard coded to a negative binomial. We don't plan to change this out in our code but it would be nice to make it clear that we could if we wanted to and to make that process fairly straightforward.

Options I see here are 1. A single name change or 2. A more complete update to either split delays from the obs model or introduce some further nesting.

function DelayObservations(delay_int,

@SamuelBrand1 in particular thoughts on which of these to consider?

@SamuelBrand1
Copy link
Collaborator

SamuelBrand1 commented Feb 27, 2024

Agreed this is a problem and connected to #29 .

My inclination is to do 1. My reason is that we are currently going with a pattern where the model decision is encoded in the type of a struct, the immutable fields of that struct (e.g. priors, delay kernel) are determined and we give that to an implementation method in a code-safe way.

Extending to Poisson observations would mean implementing (say) DelayObservationsPoisson, and a new generate_observations method. Its a bit boilerplate-y but I don't think it would be that bad. Because its such a common choice I'd write a high level constructor function, which either made a DelayObservationsNegBin or ``DelayObservationsPoisson` struct according to some option argument.

@SamuelBrand1
Copy link
Collaborator

e.g.

function DelayObservations(delay_dist; D_obs, sampling_dist = :negbin, dt = 1.0)
@assert sampling_dist is one of our two options
...double c discretisation...
K = make_observation_kernel(...)
if sampling_dist== :negbin
   ...
   return DelayObservation(...)
else
  ...
  return DelayObservationPoisson(...)

end

@SamuelBrand1
Copy link
Collaborator

Its surprisingly fiddly swapping between the two IMO due to going from one param to two param families.

I think meta-maths reason because Poisson is an exponential family distribution and Neg bin is only when $r$ parameter is fixed (e.g. a one parameter family version of Neg Bin).

@seabbs
Copy link
Collaborator Author

seabbs commented Feb 28, 2024

I don't really love the above suggestion to be honest - it doesn't seem to fit the composability spirit.

We currently have two components in the delay observation model. The delay and the observation model. Can we decompose this further and so get around this issue?

@SamuelBrand1
Copy link
Collaborator

Yes, that could work.

What we'd want from the Delay bit of the model is something that maps latent infections into expected observations. I think for 99% of use-cases this can just implemented by a (deterministic) function; but this could be random. For example, if actually modelling discrete infections the delay might be multinomial sampling on a delay distribution for each individual infection.

The implementation of Observation bit is then easy; it always expects a time series of averages and depending on the exact model it might have dispersion parameters to sample etc. In the discrete numbered infections above the Observation implementation would just be the identity map; the "expected" observations are the actual observations.

@seabbs
Copy link
Collaborator Author

seabbs commented Feb 28, 2024

Yes agree. I'll have a go at this and ping you for review in a bit

@seabbs seabbs self-assigned this Feb 28, 2024
@seabbs seabbs added this to the EpiAware 0.1.0 milestone Feb 29, 2024
@seabbs
Copy link
Collaborator Author

seabbs commented Feb 29, 2024

Thinking more about this how does #51 fit into this pattern. It could either be considered a reporting process and so deserve its own submodel or be nested in one of the other models (likely the Observation submodel).

I think I favour having the following submodels for the observation process:

  • LatentDelayModel (implying unobserved)
  • ReportingModel (i.e day of week effects, hazards etc)
  • ObservationModel (the actual observation model).

@seabbs
Copy link
Collaborator Author

seabbs commented Mar 5, 2024

This is blocked whilst we think about how to pass around the overall meta-model (which currently lacks an issue).

@seabbs
Copy link
Collaborator Author

seabbs commented Mar 7, 2024

We now have the meta model thinking (in #119 and #122) but maybe don't have clarity on this issue.

Maybe its as simple as renaming what we have to DelayNegativeBinomial and making it very functionally modular internally + documenting (or even implementing) another observation model flavour?

Or do we want to go down the route of more submodels? Or the more complex route of nesting submodels inside the observation process (Which is perhaps the most principled method?).

@SamuelBrand1 also suggested using instances of the overall model constructed nested within another model constructor where elements of the models have been conditioned on. That seems complex and hard for me to picture but might be the truly more composable way to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants