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

Issue #221: Add marginal model #426

Merged
merged 62 commits into from
Dec 3, 2024
Merged

Issue #221: Add marginal model #426

merged 62 commits into from
Dec 3, 2024

Conversation

athowes
Copy link
Collaborator

@athowes athowes commented Nov 7, 2024

Description

This PR implements the marginal model as in #221.

  • The file marginal_model-scratch.R implements a prototype outside of functions
  • This prototype is implemented into the standard structure
  • Uses primarycensored function primarycensored_lpmf which is passed into via a wrapper in marginal_model/functions.stan
    • Uses only the analytic versions of this distribution
  • Adds tests for new marginal_model

Possible steps forward here:

  • But uses non vectorised version of LPMF: want to extend to be using vectorised version of LPMF
  • Could want to extend to being able to use non analytic solution in a natural way

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

inst/cohort-scratch.R Outdated Show resolved Hide resolved
inst/cohort-scratch.R Outdated Show resolved Hide resolved
@athowes athowes changed the title Issue #221: Add cohort model Issue #221: Add marginal model Nov 14, 2024
@athowes athowes force-pushed the cohort-model branch 2 times, most recently from a415d16 to aa24b86 Compare November 15, 2024 21:04
@athowes

This comment was marked as off-topic.

@athowes

This comment was marked as off-topic.

@athowes

This comment was marked as outdated.

@epinowcast epinowcast deleted a comment from codecov bot Dec 2, 2024
@seabbs
Copy link
Contributor

seabbs commented Dec 3, 2024

@athowes there are a few bows to tie here and I am investigating the CI issues on mac and windows (it looks likes its also on main so presumably unrelated) but I think this is at the point its worth taking a look at. See https://github.com/epinowcast/epidist/actions/runs/12112515981/job/33766065135 for the same failure on main.

Updates in bullets:

  • Added a marginalised likelihood model based on primarycensored. This can be specified using as_epidist_marginal_model(). This is currently limited to Weibull, Log-Normal, and Gamma distributions with uniform primary censoring but this will be generalised in future releases. This is waiting on A lookup in R to find the stan number associated with a probability distribution primarycensored#175.
  • Added a transform_data s3 method to allow for data to be transformed for specific models. This is specifically useful for the marginal model at the moment as it allows reducing the data to its unique strata.
  • Switched over to using the marginal model everywhere
  • Added helper functions for new variables to avoid code duplication in vignettes
  • Added a heuristic for the marginal model that adapts the relative observation time to be Inf when it is much longer than the maximum delay. This is required as otherwise there are numerical instability errors from try to divide by something very near one (i.e the cdf at long observation times). In primarycensored this is a warning but here with more naive users I think it makes sense to make an informed choice and flag that that has happened.

@seabbs seabbs marked this pull request as ready for review December 3, 2024 10:23
seabbs

This comment was marked as outdated.

seabbs
seabbs previously approved these changes Dec 3, 2024
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

LGTM (self-review)

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
R/marginal_model.R Show resolved Hide resolved
R/marginal_model.R Show resolved Hide resolved
inst/stan/latent_model/functions.stan Outdated Show resolved Hide resolved
tests/testthat/setup.R Outdated Show resolved Hide resolved
tests/testthat/test-marginal_model.R Show resolved Hide resolved
vignettes/ebola.Rmd Show resolved Hide resolved
vignettes/faq.Rmd Show resolved Hide resolved
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Following comments from @athowes a follow review from me. LGTM so merging

@seabbs seabbs merged commit ce41f42 into main Dec 3, 2024
8 of 10 checks passed
@seabbs seabbs deleted the cohort-model branch December 3, 2024 13:22
@seabbs seabbs mentioned this pull request Dec 5, 2024
seabbs added a commit that referenced this pull request Jan 10, 2025
* Add cohort model template

* Fix to previous commit (name as cohort_model)

* Generate simulated cohort data

* Add unweighted and weighted direct models

* Thinking about custom family for pcd function

* functions component of stanvars

* Add transformed parameters for cohort model

* Progress on implementing PCD model

* Set q as vector

* Get rid of params input

* This would work, apart from it's the CDF. For the PMF need to import many primarycensored functions..

* Almost working with "import all functions" strategy

* Wrap up on attempt

* Rename to marginal model

* Move towards single wrap function with others imported

* Just running into some C++ errors..

* This doesn't change anything

* Move to marginal model name and lint

* Create aggregate data inside model conversion function for now

* First draft on moving marginal_model into functions

* Run document

* Tests working up to valid Stan code

* Regex version of marginal model

* Use prep_marginal_obs

* Improve assert for marginal model

* Add pkgdown and document

* Clean up scratch implementation

* remove scratch file

* update data format, formula, and family

* update stan code

* basic working version

* add transformm data methods

* start exploring the ebola example

* add a helper to find meaningful relative_obs_times

* get the full ebola vignette working with new variable requirements

* improve return messages

* update approx vignette

* get ebole vignette passing by checking pp and related inputs

* add marginal model integration tests

* expand post processing tests

* add marginal model

* use the right transform data keyword

* add ... pass through to make constructors correct

* fix .summarise_n_by_formula test so error message is as expected

* drop not required .row_id

* check using ... properly

* make the progress messages prettier for reducing data complexit:

* check post process tests again

* add a test for the specific transform data method

* add some tests for the generic transform data method

* put transform data tests in the correct folder

* add a news update

* change vignette language to talk about marginal model

* update the FAQ to use the marginal variables

* call it transformed_data not trans_data

* change the error message to make it clear its a epidist limitation

* update stan docs

* Update NEWS.md

* Update NEWS.md

Co-authored-by: Adam Howes <[email protected]>

* Update NEWS.md

Co-authored-by: Adam Howes <[email protected]>

* Update inst/stan/latent_model/functions.stan

Co-authored-by: Adam Howes <[email protected]>

* Update setup.R

---------

Co-authored-by: Sam <[email protected]>
Former-commit-id: 0c41c481bcd9767252b562e8fa704e6b43995070 [formerly 267310c44261cc0e4cae488103b7f2028b4f0153]
Former-commit-id: 277996547d66fd5d6260710b620f67f7ed3475d6
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.

2 participants