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

Change "back-calculation" to "non-mechanistic" infection model throughout #523

Open
jamesmbaazam opened this issue Nov 28, 2023 · 17 comments

Comments

@jamesmbaazam
Copy link
Contributor

At some point we should change back-calculation to nonparametric infection model throughout as that's what we're calling it in the model definition vignette.

Originally posted by @sbfnk in #521 (comment)

@sbfnk sbfnk moved this to Todo in EpiNow2 v1.5.0 Feb 20, 2024
@sbfnk sbfnk changed the title Change "back-calculation" to "nonparametric" infection model throughout Change "back-calculation" to "non-mechanistic" infection model throughout Feb 27, 2024
@sbfnk
Copy link
Contributor

sbfnk commented Mar 19, 2024

To be revisited after #345

@sbfnk sbfnk removed this from EpiNow2 v1.5.0 Mar 19, 2024
@sbfnk sbfnk added this to the CRAN v1.7 release milestone Oct 19, 2024
@jamesmbaazam
Copy link
Contributor Author

To confirm, this is just a simple re-phrasing exercise right?

@sbfnk
Copy link
Contributor

sbfnk commented Dec 17, 2024

Yes I think so (only additional thing being gradual deprecation of backcalc_opts()).

@seabbs
Copy link
Contributor

seabbs commented Dec 19, 2024

Are we sure non-mechanistic is the right name?

infections = infections + shifted_cases .* exp_noise;

The model is really a few things right:

  1. A pretty standard deconvolution approach but in the Bayesian context
  2. An exponential growth model
  3. A log link model (this is closest to being a "non-mechanistic model" in work with @SamuelBrand1 we call this a direct infections model)

One option could be to break those things up either with or without an overarching naming wrapper

@seabbs
Copy link
Contributor

seabbs commented Dec 19, 2024

Looking into this in the using the estimate_infections vignette we call this non-parametric and in the model definition vignette we call it non-mechanistic

@sbfnk
Copy link
Contributor

sbfnk commented Jan 9, 2025

The way I see it the key distinction from the default model is that it doesn't use the renewal equation so if we call that the mechanism (for generating new infections) it makes it non-mechanistic. We could call it "non-renewal" instead to be more explicit?

@jamesmbaazam
Copy link
Contributor Author

Non-renewal

Nice!

@seabbs
Copy link
Contributor

seabbs commented Jan 9, 2025

One option would be a larger breaking change to make the interface:

For renewal

estimate_infections(...
  model = rt_opts()
...
)

For deconvolution

estimate_infections(
	model = deconvolution_opt()

For growth rate

estimate_infections(
	model = growth_opts()
)

@sbfnk
Copy link
Contributor

sbfnk commented Jan 10, 2025

I really like this idea - for names perhaps we could use

estimate_infections(...
  model = renewal_opts()
...
)

instead of rt_opts()?

For growth rate

estimate_infections(
	model = growth_opts()
)

We never implemented this as it's the same as renewal with gt = Fixed(1).

@sbfnk
Copy link
Contributor

sbfnk commented Jan 10, 2025

Noting that the original issue was about wording in the docs so whilst obviously related the change of options might be better suited to a separate issue/PR combo.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 10, 2025

non-renewal

On reflection I think this is the most explicit but it's not what people will search for or be familiar with when choosing this model. That would be an argument of calling it "deconvolution".

@seabbs
Copy link
Contributor

seabbs commented Jan 10, 2025

My point was really I would be favour of closing this as a not do and update the interface instead.

We never implemented this as it's the same as renewal with gt = Fixed(1).

also for another issue but this would open up having syntax sugar to make this more visible.

@sbfnk
Copy link
Contributor

sbfnk commented Jan 10, 2025

My point was really I would be favour of closing this as a not do

I think we still want to have consistent wording in the doc (which is what the original issue was) - if we change the interface to use deconvolution_opts() and call it backcalculation in the docs I think it'll just be confusing.

@seabbs
Copy link
Contributor

seabbs commented Jan 10, 2025

yes but changing the interface changes the naming as there wont be two binary categories anymore

@sbfnk
Copy link
Contributor

sbfnk commented Jan 29, 2025

sorry, I'm not following - isn't it still two categories of renewal_model() vs. deconvolution_model() so we should update the language in the vignettes etc. accordingly?

@sbfnk
Copy link
Contributor

sbfnk commented Jan 30, 2025

pushing this to the next release as it's turning into a breaking change and we already have enough things to deprecate.

@seabbs
Copy link
Contributor

seabbs commented Jan 30, 2025

L> so we should update the language in the vignettes etc. accordingly?

because there is more than one model in the decovolution approach. I think this is all getting a bit lost. Essentially we need to make the docs match the code and as a follow up issue consider a different interface where we nest models less and each has its own opts function

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

No branches or pull requests

3 participants