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.

Sign up for GitHub

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

Jump to bottom

Add group by option to summary.pop_data() #136

Merged
merged 45 commits into from
Jul 15, 2024
Merged

Add group by option to summary.pop_data() #136

merged 45 commits into from
Jul 15, 2024

Conversation

chrisorwa
Copy link
Collaborator

@chrisorwa chrisorwa commented May 30, 2024 •
edited
Loading

Provide a strata argument to the summary.pop_data() function to allow result stratification.

@chrisorwa chrisorwa requested a review from d-morrison May 30, 2024 21:01
d-morrison
d-morrison requested changes May 31, 2024
View reviewed changes
Copy link
Member

@d-morrison d-morrison left a comment

Choose a reason for hiding this comment

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

Looks good overall; see comments.

R/summary.pop_data.R Outdated Show resolved Hide resolved
R/summary.pop_data.R Outdated Show resolved Hide resolved
@chrisorwa chrisorwa requested a review from d-morrison May 31, 2024 06:01
d-morrison
d-morrison requested changes May 31, 2024
View reviewed changes
Copy link
Member

@d-morrison d-morrison left a comment

Choose a reason for hiding this comment

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

Coming along; please see comments, and please add some unit testing for summary.pop_data()

R/summary.pop_data.R Outdated Show resolved Hide resolved
R/summary.pop_data.R Outdated Show resolved Hide resolved
R/summary.pop_data.R Outdated Show resolved Hide resolved
R/summary.pop_data.R Outdated Show resolved Hide resolved
@chrisorwa chrisorwa requested a review from d-morrison June 4, 2024 06:43
d-morrison
d-morrison requested changes Jun 6, 2024
View reviewed changes
Copy link
Member

@d-morrison d-morrison left a comment

Choose a reason for hiding this comment

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

coming along; please see notes.

R/summary.pop_data.R Outdated Show resolved Hide resolved
R/summary.pop_data.R Show resolved Hide resolved
tests/testthat/test-summary.pop_data.R Outdated Show resolved Hide resolved
@chrisorwa
Copy link
Collaborator Author

@d-morrison I have renamed the nlm_exit_codes file to data_objects and included other objects that should be on R/sysdata.rda . The reason is to append all the objects appended to R/sysdata.rda - running separate scripts and appending the results is impossible.

On the test-summary.pop_data, I have suppressed the warnings since it seems it not related to the functions - I rewrote the function to escape use of select() yet the warning persisted.

@chrisorwa chrisorwa requested a review from d-morrison July 9, 2024 06:50
d-morrison
d-morrison requested changes Jul 9, 2024
View reviewed changes
Copy link
Member

@d-morrison d-morrison left a comment •
edited by chrisorwa
Loading

Choose a reason for hiding this comment

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

Thanks for this work! Almost there. Please:

data-raw/nlm_exit_codes.R Outdated Show resolved Hide resolved
tests/testthat/test-est.incidence-status.R Outdated Show resolved Hide resolved
data-raw/typhoid_results.qmd Outdated
@@ -92,7 +73,10 @@ typhoid_results <- fit %>%
ageCat = NULL,
antigen.iso = paste(collapse = "+", "HlyE_IgG")
) %>%
structure(noise.parameters = cond.hlye.IgG)
structure(noise.parameters = noise)

usethis::use_data(typhoid_results, internal = TRUE, overwrite = TRUE)
Copy link
Member

Choose a reason for hiding this comment

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

try again? works for me.

data-raw/typhoid_results.R Outdated
Copy link
Member

Choose a reason for hiding this comment

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

please replace all manual fixtures with automated snapshots:
https://r-pkgs.org/testing-basics.html#sec-snapshot-tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is done.

d-morrison
d-morrison requested changes Jul 12, 2024
View reviewed changes
Copy link
Member

@d-morrison d-morrison left a comment

Choose a reason for hiding this comment

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

Almost there! Please see comments below.

tests/testthat/test-est.incidence.R Outdated Show resolved Hide resolved
R/sysdata.rda Outdated Show resolved Hide resolved
tests/testthat/fixtures/typhoid_results.rds Outdated Show resolved Hide resolved
data-raw/typhoid_results.qmd Outdated Show resolved Hide resolved
@lokune lokune requested a review from d-morrison July 15, 2024 08:56
d-morrison
d-morrison approved these changes Jul 15, 2024
View reviewed changes
Copy link
Member

@d-morrison d-morrison left a comment

Choose a reason for hiding this comment

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

Looks great - thanks for all your hard work on this PR!

@d-morrison d-morrison merged commit 662c265 into main Jul 15, 2024
9 checks passed
@d-morrison d-morrison deleted the add-group_by-option-to-summary branch July 15, 2024 16:50
@chrisorwa
Copy link
Collaborator Author

Cheers!!!

@d-morrison d-morrison added this to the v2.0 release milestone Jul 15, 2024
@d-morrison d-morrison linked an issue Jul 15, 2024 that may be closed by this pull request
add group_by option to summary() for class pop_data #112
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-morrison d-morrison d-morrison approved these changes

Assignees

@chrisorwa chrisorwa

Labels
None yet
Projects
None yet
Milestone
v2.0 release
Development

Successfully merging this pull request may close these issues.

add group_by option to summary() for class pop_data
2 participants
@chrisorwa @d-morrison