From 11bf8ce1e80a965c5dbcf5c56ea3923ecc1ab05a Mon Sep 17 00:00:00 2001 From: Renata Diaz Date: Tue, 28 Nov 2023 12:42:06 -0700 Subject: [PATCH] Comments and remove old test expectations. --- R/community_generate.R | 6 +++--- review_edits.md | 15 +++++++++++++++ tests/testthat/test-06_community_generate.R | 10 ---------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/R/community_generate.R b/R/community_generate.R index 29436d2..c0ec934 100644 --- a/R/community_generate.R +++ b/R/community_generate.R @@ -31,7 +31,7 @@ community_generate <- function(community_data_table, abundance_column_name = "sp community_vars <- colnames(community_data_table) - # Check that the necessary variables are provided + # Check that the necessary variables are provided #### contains_AOU <- "AOU" %in% community_vars contains_scientific_name <- "scientific_name" %in% community_vars @@ -46,7 +46,7 @@ community_generate <- function(community_data_table, abundance_column_name = "sp stop("At least one of `AOU`, `scientific_name`, or `mean_size` is required") } - # Identify ID/grouping columns and columns to pass to sim fxns. + # Identify ID/grouping columns and columns to pass to sim fxns. #### community_data_table$rejoining_id <- seq_len(nrow(community_data_table)) @@ -62,7 +62,7 @@ community_generate <- function(community_data_table, abundance_column_name = "sp sim_vars <- c(community_vars_mod[which(community_vars_mod %in% possible_sim_vars)]) - # # For the cols to pass in, add NA columns for any of the variables that the sim fxns can use that aren't included + # For the cols to pass in, add NA columns for any of the variables that the sim fxns can use that aren't included #### na_vars <- possible_sim_vars[which(!(possible_sim_vars %in% community_vars_mod))] na_table <- matrix(nrow = nrow(community_data_table), ncol = length(na_vars)) diff --git a/review_edits.md b/review_edits.md index 0cbe88a..55c971a 100644 --- a/review_edits.md +++ b/review_edits.md @@ -328,3 +328,18 @@ I've also added sections right at the beginning of both the README and Getting S - tests fail due to an issue with unzip on windows. 👀 - I've reached out to Dr. Dunning and he agreed to be listed as a Data Contributor. I've added him with this revision. - I reviewed the Small Fixes PR from @ mstrimas. Because of the scope of revisions, I ended up not merging that pull request and instead implementing the same changes on the revised codebase. Specifically, I tidied up the DESCRIPTION using desc::desc_normalize(), removed `remotes::install_github` from the README, switched T/F to TRUE/FALSE, and switched 1:nrow() to seq_len(nrow()). = was changed to <- via the lintr cleanup, and use of .data$ was removed with the removal of tidyverse functions. + +# From editor: + +* I'd recommend running desc::desc_normalize(): it will order DESCRIPTION fields in a standard way and order dependencies alphabetically. _Done!_ +* You can remove the {remotes} installation instructions and keep the {devtools} ones only as {devtools} calls {remotes} (might call {pak} in the future?) and is the interface for users. _Done!_ +* It'd be nice to add grouping to the reference index https://pkgdown.r-lib.org/reference/build_reference.html#reference-index Given your package's naming scheme, you might want to use the starts_with() helper. _I haven't done this because there are so few functions, it seems to me like it verges on redundant. I'm happy to revisit this!_ +* In the test filenames, why use numbers? If the R files and test files have the same basename, it's easier to navigate between the two in RStudio IDE. https://r-pkgs.org/testing-basics.html#test-organisation _Ah, I learned to use the numbers so that tests fail informatively in order. The tests also don't - all - correspond directly to RScripts. Is this a major barrier? If so, I'm happy to revisit!_ +* In your test files and scripts I see a lot of vertical space, more than one empty lines between some elements. I'd recommend using one empty line only as it increases the amount of code one can see at a time on the screen. It's still nice to organize the code in paragraphs, I am not suggesting to remove all empty lines. 😁 _I think this has been largely addressed via the re-styling I've done, but please let me know if the issue peresists!_ +* In the README instead of "Package summary" as a header you could use the same text as this issue "birdsize: Estimate avian body size distributions" which is more informative. _Done!_ +* Regarding data yes it might make sense to contact the author of the included dataset? _Done! I've reached out and Dr. Dunning agreed to be listed as a contributor. I've sent a more recent-follow up with the revisions and giving him the opportunity to confirm, and haven't heard back yet._ +* For comments such as https://github.com/diazrenata/birdsize/blob/7469df457989a9016ecc3761b0dd125497a3be51/R/simulate_community.R#L51 if you add 4 hyphens afterwards ----, the comment will appear in the script outline on the right in RStudio IDE (if you use that IDE), helping code navigation. https://blog.r-hub.io/2023/01/26/code-comments-self-explaining-code/#use-comments-for-the-scripts-outline _I've added these to the long community_generate script_. +* why have this "trivial" test file? https://github.com/diazrenata/birdsize/blob/main/tests/testthat/test-01_trivial.R _I use this to test that CI is working, but it's not needed now the package is built. Deleted!_ +* in test code you don't need to write birdsize::: as testthat loads your package code. Example https://github.com/diazrenata/birdsize/blob/7469df457989a9016ecc3761b0dd125497a3be51/tests/testthat/test-02_included_data.R#L13 _These are removed!_ +* why are some expectations commented out? https://github.com/diazrenata/birdsize/blob/7469df457989a9016ecc3761b0dd125497a3be51/tests/testthat/test-07_simulate_community.R#L34 _These were vestigial, removed!_ + diff --git a/tests/testthat/test-06_community_generate.R b/tests/testthat/test-06_community_generate.R index ad7f322..cc50b44 100644 --- a/tests/testthat/test-06_community_generate.R +++ b/tests/testthat/test-06_community_generate.R @@ -27,10 +27,8 @@ test_that("simulation from AOU works", { set.seed(22) bbs_data_sims <- community_generate(short_bbs_data) - # expect_true(ncol(bbs_data_sims) == 23) expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$speciestotal)) expect_false(anyNA(bbs_data_sims)) - # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 121, 120, 119))) }) test_that("simulation from AOU works with nonstandard abund name", { @@ -44,10 +42,8 @@ test_that("simulation from AOU works with nonstandard abund name", { set.seed(22) bbs_data_sims <- community_generate(short_bbs_data, abundance_column_name = "abund") - # expect_true(ncol(bbs_data_sims) == 23) expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$abund)) expect_false(anyNA(bbs_data_sims)) - # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 121, 120, 119))) expect_true(all(bbs_data_sims$sd_method == "AOU lookup")) }) @@ -63,10 +59,8 @@ test_that("simulation from species name works", { set.seed(22) bbs_data_sims <- community_generate(short_bbs_data) - # expect_true(ncol(bbs_data_sims) == 23) expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$speciestotal)) expect_false(anyNA(bbs_data_sims)) - # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 121, 120, 119))) expect_true(all(bbs_data_sims$sd_method == "Scientific name lookup")) }) @@ -85,11 +79,9 @@ test_that("simulation from mean size works", { set.seed(22) bbs_data_sims <- community_generate(short_bbs_data) - # expect_true(ncol(bbs_data_sims) == 23) expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$speciestotal)) expect_true(all(is.na(bbs_data_sims$AOU))) expect_true(all(is.na(bbs_data_sims$scientific_name))) - # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 127, 121, 117))) expect_true(all(bbs_data_sims$sd_method == "SD estimated from mean")) }) @@ -110,11 +102,9 @@ test_that("simulation from mean and sd size works", { set.seed(22) bbs_data_sims <- community_generate(short_bbs_data) - # expect_true(ncol(bbs_data_sims) == 23) expect_true(nrow(bbs_data_sims) == sum(short_bbs_data$speciestotal)) expect_true(all(is.na(bbs_data_sims$AOU))) expect_true(all(is.na(bbs_data_sims$scientific_name))) - # expect_true(all(round(bbs_data_sims$individual_mass[1:5]) == c(5824, 7147, 121, 120, 119))) expect_true(all(bbs_data_sims$sd_method == "Mean and SD provided")) })