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 93.63636% with 7 lines in your changes missing coverage. Please review.

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

Files with missing lines	Coverage Δ
R/check_strata.R	`100.00% <100.00%> (ø)`
R/utils.R	`30.76% <ø> (+4.10%)`	⬆️
R/est.incidence.by.R	`94.52% <89.55%> (+94.52%)`	⬆️

... and 3 files with indirect coverage changes

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.

…ications

…culator into simplifications

…ications

d-morrison

Thanks for this work; please see comments below and suggested edits in #245

d-morrison · 2024-11-04T19:50:42Z

NEWS.md

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


see comment in previous review; this change still needs to be reverted

R/est.incidence.by.R

Co-authored-by: Douglas Ezra Morrison <demorrison@ucdavis.edu>

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

…ications

more edits to est.incidence.by()

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

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

chrisorwa and others added 2 commits August 27, 2024 11:37

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

757676e

in progress

13be640

d-morrison requested changes Sep 9, 2024

View reviewed changes

d-morrison added 14 commits November 5, 2024 15:46

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

3ae6bed

more linting

201853d

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

c83e0cc

…ications

lint

266bae1

Merge branch 'simplifications' of https://github.com/UCD-SERG/serocal…

c3d18bf

…culator into simplifications

new test to hopefully complete patch coverage

2587245

skip parallel cores test if only one core available

c3dfcbd

added test of verbose output

6fba6a8

cleanup

2e395ad

test verbose and multicore

491cda6

more checks

8742288

cleaning up time output

23a8baa

fixes

5c98bd5

formatting

1a73533

d-morrison mentioned this pull request Nov 6, 2024

more edits to est.incidence.by() #245

Merged

d-morrison added 2 commits November 5, 2024 22:46

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

ae4fc68

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

d99d4da

…ications

d-morrison requested changes Nov 6, 2024

View reviewed changes

chrisorwa and others added 12 commits November 6, 2024 13:13

Update R/est.incidence.by.R

cf0070b

Co-authored-by: Douglas Ezra Morrison <demorrison@ucdavis.edu>

Update R/est.incidence.by.R

33dc191

Co-authored-by: Douglas Ezra Morrison <demorrison@ucdavis.edu>

additional changes

c984c18

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

9122ae8

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

change version number

177d334

make changes to NEWS.md

f953b3b

Increment version number to 1.2.0.9022

8dc73c8

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

15a32d8

…ications

Merge pull request #245 from UCD-SERG/simplifications

15f2bc2

more edits to est.incidence.by()

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

4727731

make changes

4e2b8e9

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

1cddf51

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add error message for missing strata #227

add error message for missing strata #227

chrisorwa commented Aug 1, 2024

codecov bot commented Aug 1, 2024 •

edited

Loading

d-morrison left a comment

d-morrison Aug 6, 2024

chrisorwa Aug 6, 2024

d-morrison Aug 6, 2024

chrisorwa Sep 18, 2024

d-morrison Aug 6, 2024

chrisorwa Sep 18, 2024

d-morrison left a comment

d-morrison Aug 12, 2024

chrisorwa Aug 13, 2024 •

edited

Loading

d-morrison Aug 12, 2024 •

edited

Loading

chrisorwa Aug 13, 2024 •

edited

Loading

d-morrison Aug 12, 2024

chrisorwa Sep 18, 2024

d-morrison Aug 12, 2024

chrisorwa Sep 18, 2024

d-morrison left a comment

d-morrison Aug 14, 2024 •

edited

Loading

chrisorwa Sep 18, 2024

d-morrison Aug 14, 2024

d-morrison Aug 14, 2024

chrisorwa Sep 18, 2024

d-morrison left a comment •

edited by chrisorwa

Loading

d-morrison left a comment

d-morrison Nov 4, 2024 •

edited

Loading

		@@ -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

d-morrison left a comment

Choose a reason for hiding this comment

d-morrison Nov 4, 2024 • edited Loading

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

d-morrison Nov 4, 2024 •

edited

Loading