CDCgov / Rt-without-renewal

https://cdcgov.github.io/Rt-without-renewal/
Apache License 2.0
14 stars 3 forks source link

Support complex composed models by preventing name collisions #252

Closed seabbs closed 2 months ago

seabbs commented 3 months ago

At the moment no matter what we do you can end up with situations where models can have name conclusions (i.e when they use a lot of latent models.

What should we do about this? #249 introduces a partial solution for observation models (that uses a prefix based on the named assigned by the user - partial as the uniqueness of this name is not enforced).

We also see this problem more generally (for example if ascertainment models are stacked and if multiple latent models are combined)

We have a few options as I see it.

The latter seems preferable to me as variables names will be shorter and easier to reason on. There may also be other options.

SamuelBrand1 commented 3 months ago

~The latter seems preferable to me as variables names will be shorter and easier to reason on. There may also be other options.~

Enforce prefix for all submodel calls so that a parameter name becomes dependent on its call stack

I agree here.

seabbs commented 3 months ago

The problem with this proposed approach is what if you have the following:

Acercertainment(Ascertainment(...))

Here both would have the same prefix and so collide. you could argue that this is not a pattern we want to support and they should rather write

Ascertainment(model, CombineLatentModels(Ascr1, ascr2))

This would make sense but would be hard to steer people towards potentially, may have implications for hierarchical modelling, and would mean that utility helper functions for constructing common ascertainment models etc couldn't be used in these settings (without modification).

SamuelBrand1 commented 3 months ago

Wouldn't the approach we are thinking off parse the top example as having a prefix like asc_asc... which is obviously ugly but distinguishable?

seabbs commented 3 months ago

Wouldn't the approach we are thinking off parse the top example as having a prefix like asc_asc... which is obviously ugly but distinguishable?

So not if we made the prefix only on the latent model vs both the latent and obs model (which is what I was suggesting). If it applies to all models then at that point you might as well have everything prefix because you will have very inconsistent behaviour depending on which structs you use.

SamuelBrand1 commented 3 months ago

In my head I was agreeing to the call stack suggestion... and C&Ped the wrong bit. Is the main downside that its really ugly?

seabbs commented 3 months ago

In my head I was agreeing to the call stack suggestion... and C&Ped the wrong bit. Is the main downside that its really ugly?

Yes really ugly and really tedious to navigate over for people manipulating the posterior output. It also makes it very hard to write code for multiple models.

seabbs commented 2 months ago

Like CombineLatentModels, ConcatLatentModels (see #270) also has this problem.

seabbs commented 2 months ago

Another option we could take here that would be less automated but maybe more informative is make defining the prefix part of the struct. This could look something like:

Ascertainment(NegativeBinomialError(), RW(), prefix = "IFR")

This could be nice as it lets the user set very clear names for things but it could also maybe cause issues as the models will error a few times (due to name collisions) and the user will need to know what to do (which maybe isn't the most fun way. to interact with a package). Some of this could be mitigated if we make a default prefix for these complex operators?

SamuelBrand1 commented 2 months ago

Yes! Instant reaction is that this is the best idea.

I think the kind of users who'd want to construct a sufficiently complex model that naming collisions come into play, probably have their own namespace ideas for the model components.

This feels like a natural solution. And the defaults should be simple to select?

seabbs commented 2 months ago

I like that it is 1. Simple and 2. Should be adaptable to all the issues we have identified above with automated strategies.

seabbs commented 2 months ago

I've started looking at implementing this here: https://github.com/CDCgov/Rt-without-renewal/blob/issue252/EpiAware/src/EpiLatentModels/manipulators/CombineLatentModels.jl

One uncertainty is if we should let the user specify the prefix prefix (i.e here Combine) or not.

A slightly annoying issue is you can't have an empty prefix ("" will add "." on everything) so this can't be turned off as a feature.

Otherwise work to do here is:

Something this really does highlight is how much the named tuple returns of everything don't work this approach (as the returns now have a different name to the variables in the model).