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

Polish getting started vignette #57

Closed
1 of 4 tasks
athowes opened this issue May 23, 2024 · 2 comments · Fixed by #59
Closed
1 of 4 tasks

Polish getting started vignette #57

athowes opened this issue May 23, 2024 · 2 comments · Fixed by #59
Assignees

Comments

@athowes
Copy link
Collaborator

athowes commented May 23, 2024

Good that we merged in the "get started" vignette and now can see it rendered: http://epidist.epinowcast.org/articles/epidist.html

There are a few small issues I can see with it that require polishing up:

  • Move loading packages to where the loading packages is mentioned
  • Alter the "Finally, in Section ??, we demonstrate that the fitted delay distribution accurately recovers the underlying truth." (I merged two sections)
  • Ideally the Table 2.1 caption should be altered to work like figure captions rather than this strange thing. I feel like this is possible but requires trying a few things out. Basically I'd like to be using (ref:...)
  • The section at the end still has "Figure 2.2 shows…" and an empty figure caption that need to be filled out

Can make a quick PR to fix these things tomorrow.

@athowes athowes self-assigned this May 23, 2024
@parksw3
Copy link
Collaborator

parksw3 commented May 23, 2024

This looks amazing. Just a few thoughts.

  1. I wonder if we should define what we mean by primary vs secondary event? Or maybe it's obvious?

  2. Should we add more citations or is Park & Charniga et al good enough?

  3. We might want to explain figure 1.4 a bit more carefully. I was slightly confused about how different delay times can have the same censored delay time. For example, if we have ptime=0 and stime=1.5, then the observed delay is 1 day. But if ptime=0.9 and stime=2.4 then the observed delay is 2 days. Something about censoring happening at the event level, rather than at the delay level...

  4. Figure 2.1: Just so I know I'm understanding this properly, do the orange bars represent full data without censoring? I wonder if it makes more sense to plot them with density plots (alongside the true density) since they're continuous data? We could also consider plotting something like the mean (vertical line) to show the bias.

@seabbs
Copy link
Contributor

seabbs commented May 24, 2024

I wonder if we should define what we mean by primary vs secondary event? Or maybe it's obvious?

Agree

Park & Charniga et al good enough?

I feel like its enough as an entry point (we could even say this explicitly?)

if it makes more sense to plot them with density plots (alongside the true density) since they're continuous data?

Agree - I think we don't then plot correctly discretised dists which we probably should.

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 a pull request may close this issue.

3 participants