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

plotting functions for coverage and survival #31

Merged
merged 64 commits into from
Oct 30, 2023
Merged

Conversation

davidsantiagoquevedo
Copy link
Member

  • Functions to plot survival
  • Functions to calculate and plot vaccine coverages
  • Current coverage: 60%

@pratikunterwegs
Copy link
Contributor

Thanks @davidsantiagoquevedo - I'll take a look at this next week and get back to you by Weds? Meanwhile perhaps you could already fix the linting.

@davidsantiagoquevedo
Copy link
Member Author

Hi @pratikunterwegs. Thanks! By Wednesday is perfect. I'll check the lining. We also have today a collaborative session at PUJ for the test coverage. Currently 60%

@pratikunterwegs
Copy link
Contributor

Thanks - I'm taking a look now, and I can take another look tomorrow in case you've added more tests for the plots. Snapshot tests should do the trick.

@davidsantiagoquevedo
Copy link
Member Author

Hello @pratikunterwegs, we fixed the errors on the checks generated by the tests. Could you please take a look at this PR when you have time?
Thanks!

@pratikunterwegs pratikunterwegs self-requested a review October 23, 2023 11:03
@pratikunterwegs
Copy link
Contributor

Hi @davidsantiagoquevedo sure, I could take a look by the end of the week if that's alright?

@davidsantiagoquevedo
Copy link
Member Author

Hello @pratikunterwegs . Yes, by the end of the week is fine. Thank you!

@davidsantiagoquevedo davidsantiagoquevedo changed the title Dev plotting functions for coverage and survival Oct 24, 2023
Copy link
Contributor

@pratikunterwegs pratikunterwegs left a comment

Choose a reason for hiding this comment

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

Nice stuff @davidsantiagoquevedo - looks good to me overall. I had only a few comments which I've made on the code.

One recommendation though would be to refactor plot_coverage() --- this function wraps coh_coverage(). I would instead let the two be chain-able, so that the output of coh_coverage() can be passed to plot_coverage(). You would need to add some input checks on data to ensure it has the expected columns.

coh_coverage(<ARGS>) |> plot_coverage()

This reduces the nestedness of the code overall, and if there are other methods (i.e., study methods) from which coverage can be calculated, these could also be handled by plot_coverage() in future.

some_method_coverage(<ARGS>) |> plot_coverage()

.github/workflows/pkgdown.yaml Outdated Show resolved Hide resolved
R/coh_plot.R Outdated Show resolved Hide resolved
tests/testthat/test-cohort_effectiveness.R Outdated Show resolved Hide resolved
tests/testthat/test-plot_survival.R Outdated Show resolved Hide resolved
@davidsantiagoquevedo davidsantiagoquevedo merged commit 453643b into main Oct 30, 2023
8 checks passed
@davidsantiagoquevedo davidsantiagoquevedo deleted the dev branch May 1, 2024 19:39
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.

3 participants