add error message for missing strata #227

chrisorwa · 2024-08-01T13:13:26Z

No description provided.

codecov · 2024-08-01T13:17:22Z

Codecov Report

Attention: Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
R/est.incidence.by.R	0.00%	17 Missing ⚠️

Files with missing lines	Coverage Δ
R/est.incidence.by.R	`0.00% <0.00%> (ø)`

d-morrison

Thanks for this PR; please address comments and add tests for the code lines in this PR.

d-morrison · 2024-08-06T07:11:38Z

R/est.incidence.by.R

+    cli::cli_abort(
+      class = "missing_var",
+      message = c(
+        "Can't find the column {.var {strata}} in {.envvar pop_data}.",


please revise this inline markup; pop_data is a function argument, not an environment variable (see https://cli.r-lib.org/reference/inline-markup.html)

This is well-noted.

d-morrison · 2024-08-06T07:14:02Z

R/est.incidence.by.R

+        if (length(strata_var) > 0) {
+          "i" <- "Did you mean {.var {strata_var}}."
+        }


please add a test that covers this case

This has been implemented.

d-morrison · 2024-08-06T07:15:24Z

R/est.incidence.by.R

@@ -101,6 +101,27 @@ est.incidence.by <- function(
    return(to_return)
  }

+  if (!any(is.element(strata,pop_data %>% names()))) {


please always format according to the style guide (add a space after the comma in strata,pop_data, etc)

The change has been made.

d-morrison

Getting closer; please see inline comments.

d-morrison · 2024-08-12T22:03:19Z

R/est.incidence.by.R

@@ -101,6 +101,27 @@ est.incidence.by <- function(
    return(to_return)
  }

+  if (!any(is.element(strata, pop_data %>% names()))) {


should any() be all()?

This should be all(). The change has been made.

d-morrison · 2024-08-12T22:07:19Z

R/est.incidence.by.R

+      grep(
+        x = pop_data %>% names(),
+        value = TRUE,
+        pattern = strata,


what happens here if strata has multiple entries in it? for example, strata = c("Country", "catch")? Please check this scenario and adjust this code if needed (probably, try to match each missing stratum variable separately).

This is in order and adds complexity to Issue #233. For example, if strata = c("age", "Country"), I had opted to use regex paste0("^", strata) to capture only columns that begin with age to avoid and avoid words such as village.

Addendum:
Never mind, I found a solution to it.

d-morrison · 2024-08-12T22:11:04Z

R/est.incidence.by.R

+      message = c(
+        "Can't find the column {.arg {strata}} in {.arg pop_data}.",
+        if (length(strata_var) > 0) {
+          "i" <- "Did you mean {.var {strata_var}}."


Please end questions with a question mark instead of a period.

This has been implemented.

d-morrison · 2024-08-12T22:12:17Z

R/est.incidence.by.R

+    cli::cli_abort(
+      class = "missing_var",
+      message = c(
+        "Can't find the column {.arg {strata}} in {.arg pop_data}.",


Please inform the user that this error relates to their input for the strata argument, so they know which input to fix.

This has been implemented.

…ithub.com/UCD-SERG/serocalculator into add-user-error-message-for-missing-strata

d-morrison

Thanks for the hard work on this PR; please see inline comments.

d-morrison · 2024-08-14T22:22:33Z

R/est.incidence.by.R

@@ -101,6 +101,36 @@ est.incidence.by <- function(
    return(to_return)
  }

+  if (!all(is.element(strata, pop_data %>% names()))) {
+    # accommodate multiple strata
+    combined_pattern <- paste(strata, collapse = "|")


This approach seems sub-optimal, for two reasons:

we won't be able to tell which columns partially match each specific missing element of strata.

we didn't remove the elements of strata that already exactly match columns in pop_data

More generally, complicated regular expressions are a recipe for bugs; if you think you need to use | in a regex, please carefully consider alternative approaches. As always, we want to keep the code as simple as possible; for example, in this case, instead of pasting strata together, we could instead individually grep each element of strata that isn't exactly in pop_data, using a for loop, or lapply(), or purrr::map(), etc.

Then, we can report the possible partial matches for each missing element of strata; the markup capabilities of cli_abort() enable us to provide this information in an orderly, structured format. Please give all this a try, but if you get stuck, just let me know and I'll provide more detailed instructions.

I have made changes to this approach to capture full match, partial match and no match.

d-morrison · 2024-08-14T22:25:37Z

tests/testthat/test-est.incidence.R

+  curve <- load_curve_params("https://osf.io/download/rtw5k/")
+
+  expect_error(
+    object = est.incidence.by(


why are we testing est.incidence.by() in the test file for est.incidence()? Please be careful to keep these tests properly organized.

please add a test in which strata contains multiple elements that don't exactly match the columns of pop_data

I have separated est.incidence and est.incidence.by into different scripts. In addition, I have added a test for multiple strata in est.incidence.by.

d-morrison

Looks like this PR is still in-progress?

Please address the lint errors and restore the files that seem to have been accidentally deleted.
Before re-requesting review, please look through Files changed and add at least one comment per changed file: briefly explain what's been changed and why.

R/est.incidence.by.R

chrisorwa · 2024-09-13T08:16:09Z

R/est.incidence.by.R

@@ -39,7 +50,7 @@
 #'
 #' curve <- load_curve_params("https://osf.io/download/rtw5k/") %>%
 #'   filter(antigen_iso %in% c("HlyE_IgA", "HlyE_IgG")) %>%
-#'   slice(1:100, .by = antigen_iso) # Reduce dataset for the purposes of this example
+#'   slice(1:100, .by = antigen_iso)


Remove comment to make the line less than 80 characters.

instead of removing the comment, why not move it to a new line?

DESCRIPTION

R/est.incidence.by.R

chrisorwa · 2024-09-17T14:12:37Z

R/est.incidence.by.R

+      require(serocalculator) # note - this gets out of sync" %>%
+      "when using load_all() in development"


Reduce sentences to 80 characters (linting requirement).

it looks like the second half of that comment got turned into code; why?

tests/testthat/_snaps/est.incidence.by.md

chrisorwa · 2024-09-17T14:58:19Z

tests/testthat/test-as_curve_params.R

This file has been moved to PR #228 . This file was originally part of the as_noise_params PR and got to this PR by mistake. I have moved it back to the noise PR.

d-morrison

Thanks for working on this! Please see comments. Not sure why codecov didn't throw an error; please increase the patch coverage.

d-morrison · 2024-09-23T23:44:21Z

DESCRIPTION

@@ -1,7 +1,7 @@
 Package: serocalculator
 Type: Package
 Title: Estimating Infection Rates from Serological Data
-Version: 1.2.0.9001
+Version: 1.3.0


please increment dev version instead of release version

d-morrison · 2024-09-23T23:45:16Z

NEWS.md

@@ -1,4 +1,7 @@
-# serocalculator (development version)
+# serocalculator 1.3.0


use dev version

d-morrison · 2024-09-23T23:46:46Z

R/est.incidence.by.R

@@ -26,7 +35,9 @@
 #'
 #' @return
 #' * if `strata` has meaningful inputs:
-#' An object of class `"seroincidence.by"`; i.e., a list of `"seroincidence"` objects from [est.incidence()], one for each stratum, with some meta-data attributes.
+#' An object of class `"seroincidence.by"`; i.e., a list of `


please move linebreak to before the backtick

d-morrison · 2024-09-23T23:48:34Z

NEWS.md

@@ -1,4 +1,7 @@
-# serocalculator (development version)
+# serocalculator 1.3.0
+* Add test for `est.incidence` 


est.incidence.by?

and what behavior is being tested?

d-morrison · 2024-09-23T23:49:52Z

NEWS.md

+# serocalculator 1.3.0
+* Add test for `est.incidence` 
+
+* Add option to test `strata` in `pop_data`


is this a user-facing option? not sure what this means.

d-morrison · 2024-09-24T00:08:14Z

tests/testthat/test-est.incidence.by.R

+    slice_head(n = 100)
+
+
+  est_true <- est.incidence(


d-morrison · 2024-09-24T00:08:21Z

tests/testthat/test-est.incidence.by.R

+    slice_head(n = 100)
+
+
+  est_false <- est.incidence(


d-morrison · 2024-09-24T00:08:47Z

tests/testthat/test-as_curve_params.R

d-morrison · 2024-09-24T00:09:43Z

tests/testthat/test-est.incidence.by.R

+        start = start
+      )
+
+    expect_snapshot(x = typhoid_results)


please add an additional test using expect_snapshot_value() or expect_snapshot_file() (please think through which one seems best to use).

d-morrison · 2024-09-24T03:10:42Z

R/est.incidence.by.R

+      "consider using the `est.incidence()` function to
+      simplify your code and avoid this warning.",
+      "\n\n Since the `strata` argument is empty, `est.incidence.by()`
+      will return a `seroincidence` object, instead of a
+      `seroincidence.by` object.\n"


please add tests everywhere indicated by Codecov comments

add error message for missing strata

88a24bd

chrisorwa linked an issue Aug 1, 2024 that may be closed by this pull request

Add user error message for missing strata #224

Open

chrisorwa added 2 commits August 1, 2024 16:45

run document() for est.incidence.by

5ec8ec0

add any() to is.element()

b82e3b5

chrisorwa requested a review from d-morrison August 5, 2024 11:11

d-morrison requested changes Aug 6, 2024

View reviewed changes

chrisorwa added 2 commits August 7, 2024 14:52

add test

3c271ee

Merge branch 'main' into add-user-error-message-for-missing-strata

ee3d7ca

chrisorwa requested a review from d-morrison August 12, 2024 08:52

d-morrison requested changes Aug 12, 2024

View reviewed changes

chrisorwa added 3 commits August 14, 2024 11:07

Merge branch 'main' into add-user-error-message-for-missing-strata

39ea75b

modify error messaging

0cb88d6

Merge branch 'add-user-error-message-for-missing-strata' of https://g…

97efcf3

…ithub.com/UCD-SERG/serocalculator into add-user-error-message-for-missing-strata

chrisorwa requested a review from d-morrison August 14, 2024 21:10

d-morrison requested changes Aug 14, 2024

View reviewed changes

d-morrison added bug an unexpected problem or unintended behavior enhancement API application programming interface (exported functions, etc) unit testing labels Aug 14, 2024

d-morrison added this to the v2.0 release milestone Aug 14, 2024

chrisorwa added 5 commits August 21, 2024 11:08

make changes to tests and est.incidence.by

b3523c7

add purrr to DESCRIPTION

c09df9b

modify error message

42c7af6

correct errors

0f36e94

remove testthat from DESCRIPTION

f97218a

chrisorwa requested a review from d-morrison August 26, 2024 11:31

Merge branch 'main' into add-user-error-message-for-missing-strata

757676e

d-morrison requested changes Sep 9, 2024

View reviewed changes

Merge branch 'main' into add-user-error-message-for-missing-strata

87625a1

chrisorwa added 3 commits September 11, 2024 00:22

lint file

3bf79b3

lint tests

927911a

Merge branch 'main' into add-user-error-message-for-missing-strata

655985b

chrisorwa commented Sep 12, 2024

View reviewed changes

R/est.incidence.by.R Show resolved Hide resolved

chrisorwa commented Sep 13, 2024

View reviewed changes

R/est.incidence.by.R Show resolved Hide resolved

chrisorwa commented Sep 17, 2024

View reviewed changes

increment version

8302735

chrisorwa commented Sep 17, 2024

View reviewed changes

chrisorwa requested a review from d-morrison September 17, 2024 15:07

chrisorwa mentioned this pull request Sep 18, 2024

improve autoplot.pop_data error communication #234

Merged

d-morrison added 2 commits September 23, 2024 16:47

Merge branch 'main' into add-user-error-message-for-missing-strata

2ef8e8e

increment version

6b365c0

d-morrison requested changes Sep 24, 2024

View reviewed changes

d-morrison added feature a feature request or enhancement and removed enhancement labels Sep 25, 2024

chrisorwa added 4 commits September 29, 2024 20:16

Merge branch 'main' into add-user-error-message-for-missing-strata

7cac80b

Merge branch 'main' into add-user-error-message-for-missing-strata

cc000f7

correct conflicts

d3a8b8c

requested changes

c22ed3c

		require(serocalculator) # note - this gets out of sync" %>%
		"when using load_all() in development"

		@@ -1,4 +1,7 @@
		# serocalculator (development version)
		# serocalculator 1.3.0

add error message for missing strata #227

Are you sure you want to change the base?

add error message for missing strata #227

Conversation

chrisorwa commented Aug 1, 2024

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

d-morrison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-morrison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisorwa Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

d-morrison Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

chrisorwa Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-morrison left a comment

Choose a reason for hiding this comment

d-morrison Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-morrison left a comment • edited by chrisorwa Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisorwa Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

d-morrison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 1, 2024 •

edited

Loading

chrisorwa Aug 13, 2024 •

edited

Loading

d-morrison Aug 12, 2024 •

edited

Loading

chrisorwa Aug 13, 2024 •

edited

Loading

d-morrison Aug 14, 2024 •

edited

Loading

d-morrison left a comment •

edited by chrisorwa

Loading

chrisorwa Sep 17, 2024 •

edited

Loading