-
Notifications
You must be signed in to change notification settings - Fork 4
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
89 refac summary methods #108
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comment I made in the full package review @davidsantiagoquevedo. I've left a few comments in the diffs. Please let me know if any of my comments are unclear and I'm happy to discuss and clarify.
Hi @joshwlambert. Thanks for your review. I addressed all the comments. Please, let me know if you have further suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
It would be good to add unit tests for the functions you've added in this PR in the future.
Small PR answering issue #89
Main changes
summary.match()
method sincematch_cohort
is not intended to be used externally and it already returns the necessary outputs to feed othersummary
methodssummary.vaccineff_data()
and addedprint.summary.vaccineff_data()
summary.vaccineff
and addedprint.summary.vaccineff()