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 37 draft: Why it works vignette with analytic solutions #68

Merged
merged 35 commits into from
Sep 18, 2024
Merged

Issue 37 draft: Why it works vignette with analytic solutions #68

merged 35 commits into from
Sep 18, 2024

Conversation

SamuelBrand1
Copy link
Collaborator

@SamuelBrand1 SamuelBrand1 commented Sep 16, 2024

Description

This PR closes #37.

This draft PR creates a "why it works" vignette for the mathematics underlying the approach to inference under censoring/truncation used in primarycensoreddist.

The basic concept is to present the primary censoring window problem in terms of the survival function of the inter-event time after the end of the primary event window. This random variable can be negative therefore instead of the survival function starting at 1, it starts at the probability that the secondary event occurs after the end of the primary censor window.

This survival function can be either the target of numerical quadrature, or it might have analytically accessible solutions.

I've started adding examples of where the survival function has analytic solutions. This is WIP:

  • Gamma with uniform primary window.
  • Log-Normal with uniform primary window.

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.

vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
@seabbs
Copy link
Contributor

seabbs commented Sep 16, 2024

Thanks for this @SamuelBrand1. From a first pass it looks great and having some analytical solutions is very exciting

To do before I've read content:

  • Run: spelling::update_wordlist()
  • Add yourself to the description as an author
  • Follow up issues for adding R and stan support for analytical solutions + testing ( I can make these issues or happy for you to do so).
  • Add the new vignette to the articles section of the pkgdown.yaml.
  • Link to it (via relative links https://pkgdown.r-lib.org/articles/linking.html)

@seabbs
Copy link
Contributor

seabbs commented Sep 16, 2024

Read to dos:

  • Consider changing the title to the very on the noise: Why it works
  • Link at the top to the getting started + provide some kind of pre intro text along the lines of here be dragons.
  • Split why it works and analytical solutions into their own vignettes (?). If we do that we might need some subheadings in the articles dropdown to guide readers

vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved

Using integration by parts gives:
$$
Q_{S_+}(t) = Q_T(t + W_P) + \int_0^{W_P} f_T(t+p) C(p) dp.
Copy link
Contributor

Choose a reason for hiding this comment

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

missing f_T def

@SamuelBrand1
Copy link
Collaborator Author

SamuelBrand1 commented Sep 18, 2024

I can't edit @seabbs check list so I'll C&P here:

  • Run: spelling::update_wordlist()
  • Add yourself to the description as an author
  • Follow up issues for adding R and stan support for analytical solutions + testing ( I can make these issues or happy for you to do so).
  • Add the new vignette to the articles section of the pkgdown.yaml.
  • Link to it (via relative links https://pkgdown.r-lib.org/articles/linking.html)
  • Consider changing the title to the very on the noise: Why it works
  • Link at the top to the getting started + provide some kind of pre intro text along the lines of here be dragons.
  • Split why it works and analytical solutions into their own vignettes (?). If we do that we might need some subheadings in the articles dropdown to guide readers

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 6120812 is merged into main:

  • ✔️cmdstan_fit_gamma: 7.69s -> 7.73s [-1.77%, +3.02%]
  • ✔️cmdstan_fit_lognormal: 1.61s -> 1.58s [-5.63%, +1.05%]
  • ✔️dprimarycensoreddist_expgrowth: 1.95ms -> 1.95ms [-1.75%, +1.92%]
  • ✔️dprimarycensoreddist_weibull: 1.8ms -> 1.81ms [-0.46%, +1.86%]
  • ✔️fitdistdoublecens_gamma: 639ms -> 639ms [-0.5%, +0.45%]
  • ✔️fitdistdoublecens_normal: 537ms -> 536ms [-1%, +0.47%]
  • ✔️pprimarycensoreddist_expgrowth: 36.3ms -> 36.2ms [-1.7%, +1.31%]
  • ✔️pprimarycensoreddist_lnorm: 33.6ms -> 33.4ms [-3.72%, +2.47%]
    Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 05daf84 is merged into main:

  • ✔️cmdstan_fit_gamma: 7.64s -> 7.65s [-0.67%, +0.87%]
  • ✔️cmdstan_fit_lognormal: 1.68s -> 1.67s [-2.72%, +1.22%]
  • ✔️dprimarycensoreddist_expgrowth: 2.12ms -> 2.02ms [-11.94%, +2.54%]
  • ✔️dprimarycensoreddist_weibull: 1.99ms -> 1.99ms [-2.15%, +1.7%]
  • ✔️fitdistdoublecens_gamma: 665ms -> 673ms [-1.4%, +3.51%]
  • ✔️fitdistdoublecens_normal: 554ms -> 551ms [-0.95%, +0.1%]
  • ✔️pprimarycensoreddist_expgrowth: 40.8ms -> 40.8ms [-1.1%, +0.71%]
  • ✔️pprimarycensoreddist_lnorm: 37.4ms -> 37.3ms [-1.4%, +0.72%]
    Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@SamuelBrand1
Copy link
Collaborator Author

My questions towards resolving final check list items:

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 9df87c0 is merged into main:

  • ✔️cmdstan_fit_gamma: 7.62s -> 7.59s [-0.95%, +0.21%]
  • ✔️cmdstan_fit_lognormal: 1.6s -> 1.56s [-6.35%, +2.21%]
  • ✔️dprimarycensoreddist_expgrowth: 1.89ms -> 1.91ms [-0.13%, +2.54%]
  • ✔️dprimarycensoreddist_weibull: 1.82ms -> 1.81ms [-2.03%, +0.39%]
  • 🚀fitdistdoublecens_gamma: 655ms -> 643ms [-3.48%, -0.35%]
  • ✔️fitdistdoublecens_normal: 541ms -> 540ms [-0.85%, +0.59%]
  • ✔️pprimarycensoreddist_expgrowth: 36.6ms -> 36.6ms [-1.53%, +1.06%]
  • ✔️pprimarycensoreddist_lnorm: 33.3ms -> 33.1ms [-1.86%, +0.69%]
    Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@seabbs
Copy link
Contributor

seabbs commented Sep 18, 2024

Where would we want to link to this?

The getting started vignette. Can be its own issue.

Split the vignette now or in a new PR to resolve

Given this has taken a while perhaps in its PR.

What is a news item?

A new line in NEWS.md that tracks changes. May be more familiar as a CHANGELOG

@seabbs seabbs marked this pull request as ready for review September 18, 2024 14:40
@seabbs
Copy link
Contributor

seabbs commented Sep 18, 2024

Interestingly I can tick your boxes but you can't tick mine. Might be about repo status. I will give you some repo permissions

@seabbs seabbs enabled auto-merge (squash) September 18, 2024 14:40
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
vignettes/why-it-works.Rmd Outdated Show resolved Hide resolved
seabbs
seabbs previously approved these changes Sep 18, 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.

This is really clear now. Nice work. I think merge and deal with other changes as their own issues.

@seabbs seabbs disabled auto-merge September 18, 2024 15:23
@seabbs seabbs merged commit bffe696 into epinowcast:main Sep 18, 2024
10 checks passed
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8059534 is merged into main:

  • 🚀cmdstan_fit_gamma: 7.71s -> 7.61s [-1.86%, -0.74%]
  • ✔️cmdstan_fit_lognormal: 1.6s -> 1.59s [-4.08%, +2.7%]
  • ✔️dprimarycensoreddist_expgrowth: 1.93ms -> 1.93ms [-1.86%, +2.43%]
  • ✔️dprimarycensoreddist_weibull: 1.84ms -> 1.85ms [-1.51%, +2.61%]
  • ✔️fitdistdoublecens_gamma: 662ms -> 660ms [-0.86%, 0%]
  • 🚀fitdistdoublecens_normal: 557ms -> 552ms [-1.44%, -0.25%]
  • ✔️pprimarycensoreddist_expgrowth: 36.5ms -> 37ms [-1.15%, +3.55%]
  • ✔️pprimarycensoreddist_lnorm: 33.7ms -> 33.5ms [-4.22%, +3.06%]
    Further explanation regarding interpretation and methodology can be found in the documentation of touchstone.

@SamuelBrand1 SamuelBrand1 deleted the why-it-works-vignette branch September 19, 2024 12:57
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.

A why it works maths vignette
2 participants