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 #64: Vignette explaining model #405

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Issue #64: Vignette explaining model #405

wants to merge 14 commits into from

Conversation

parksw3
Copy link
Collaborator

@parksw3 parksw3 commented Oct 22, 2024

Description

This draft PR will close #64.

[Describe the changes that you made in this pull request.]

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.

@parksw3 parksw3 closed this Oct 22, 2024
@parksw3 parksw3 reopened this Oct 22, 2024
@parksw3
Copy link
Collaborator Author

parksw3 commented Oct 22, 2024

I can't figure out how to make draft PR...

@athowes
Copy link
Collaborator

athowes commented Oct 22, 2024

"Mark as draft" in bottom right.

image

@athowes athowes marked this pull request as draft October 22, 2024 14:07
@athowes athowes changed the title Model vignette Issue ##64: Vignette explaining model Oct 22, 2024
@parksw3
Copy link
Collaborator Author

parksw3 commented Oct 22, 2024

Ah ok thank you.

@athowes athowes changed the title Issue ##64: Vignette explaining model Issue #64: Vignette explaining model Oct 22, 2024
@seabbs
Copy link
Contributor

seabbs commented Oct 22, 2024

Don't forget when reading folksyou can get the HTML version in the details section of the pkgdown check

@seabbs
Copy link
Contributor

seabbs commented Oct 22, 2024

Thanks @parksw3 looks great to me. I'm going to take a back seat here but I think the key questions for others are:

  1. Correctness
  2. Is this the right amount of detail
  3. Is this communicated at the right level for the audience of this package and if they need other resources are those clearly communicated
  4. Can we easily extend this setup for new methods as they come along

@SamuelBrand1
Copy link

First quick thing: can this get added to the article here get added to the article list in _pkgdown.yml?

@athowes
Copy link
Collaborator

athowes commented Oct 23, 2024

First quick thing: can this get added to the article here get added to the article list in _pkgdown.yml?

Agree, it's an edit like this:

- text: Approximate Bayesian inference

 - text: Guide to epidist models
 - href: articles/model.html

@seabbs
Copy link
Contributor

seabbs commented Oct 23, 2024

Can someone just suggest the yaml edit to speed this up

@athowes
Copy link
Collaborator

athowes commented Oct 25, 2024

I agree with Sam that ideally we'd have some kind of extensible structure here for new models. I don't think we have that yet. At the moment things are laid out giving the required background to understand the models. Perhaps what we want to do is to move everything that goes into "the perfect model" up to background, and then have new sections for each approximation and explain at what point in the perfect model they form an approximation. We also have the direct_model now to do this for.

At the moment I also don't think we have sign posting to many resources, aside from Park et al. and Ward et al. But I'm not sure what else would be useful.

&= \int_{P_L}^{P_R} \int_{S_L}^{S_R} g_P(x\,|\,P_L,P_R) f_x(y-x) \,dy\, dx
\end{aligned}
$$
where $ g_P(x\,|\,P_L,P_R)$ represents the conditional distribution of primary event given lower $P_L$ and upper $P_R$ bounds.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
where $ g_P(x\,|\,P_L,P_R)$ represents the conditional distribution of primary event given lower $P_L$ and upper $P_R$ bounds.
where $g_P(x \, | \, P_L, P_R)$ represents the conditional distribution of primary event given lower $P_L$ and upper $P_R$ bounds.

@seabbs
Copy link
Contributor

seabbs commented Nov 22, 2024

yo yo yo what needs to happen to get this moving folks :)

@parksw3
Copy link
Collaborator Author

parksw3 commented Nov 22, 2024

I can take another stab at this over the weekend & next week if someone gives more precise instructions for what needs to be done. Maybe I'll try to separate the writing into backgrounds, "perfect model", and approximations, based on @athowes's suggestions meanwhile before anyone does that (but would still appreciate instructions/suggestions)...

I can also try and see if I can figure out yaml edit

@parksw3
Copy link
Collaborator Author

parksw3 commented Nov 25, 2024

I've made minor modifications to text and formatting based on @athowes's earlier comments. I'm happy to do more editing/writing this week. Just let me know what needs to be done.

And I think I correctly added this vignette to _pkgdown.yml?

https://github.com/epinowcast/epidist/blob/041ca516d670d13f4545ce5f8049123dd2a7358e/_pkgdown.yml#L23C1-L24C34

@athowes
Copy link
Collaborator

athowes commented Nov 25, 2024

Thanks @parksw3! Have added the _pkgdown.yml bit (933df50) and doing a rebase plus clean up of the tests / precommit.

I've added an unpopulated section on the naive model which we should fill in. I think before merge the other sections probably need some editing for readability at a wider audience. Some of what we have at the moment is just me writing down things in quick notes.

@parksw3
Copy link
Collaborator Author

parksw3 commented Nov 25, 2024

I can fill out naive model and spend some time editing for readability. Just to clarify, the naive model is the model without truncation or censoring?

@athowes
Copy link
Collaborator

athowes commented Nov 26, 2024

I can fill out naive model and spend some time editing for readability.

Yes that would be great, thank you!

Just to clarify, the naive model is the model without truncation or censoring?

Yes that's correct. It is just modelling a distribution on the delay directly. The code for it is here (very minimal, just basically passing delay ~ ... through to brms).

We think having this model will be useful for teaching e.g. #403 and also in papers (like yours) to have something to compare the latent or marginal model to.

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.

Vignette explaining model
4 participants