refactor: clean up error messages #413

dshemetov · 2024-02-01T01:45:58Z

Checklist

Please:

Make sure this PR is against "dev", not "main".
Request a review from one of the current epiprocess main reviewers:
brookslogan, nmdefries.
Makes sure to bump the version number in DESCRIPTION and NEWS.md.
Always increment the patch version number (the third number), unless you are
making a release PR from dev to main, in which case increment the minor
version number (the second number).
Describe changes made in NEWS.md, making sure breaking changes
(backwards-incompatible changes to the documented interface) are noted.
Collect the changes under the next release number (e.g. if you are on
0.7.2, then write your changes under the 0.8 heading).

Change explanations for reviewer

Removed Abort and Warn, replaced with direct calls to cli functions
Replaced many custom arg validation statements with checkmate equivalents
Updated test regexp expectations

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

Resolves Clean up error messages and assertions #380

dajmcdon · 2024-02-01T15:37:28Z

R/archive.R

-        }
+        assert_data_frame(x)
+        assert_subset(c("geo_value", "time_value", "version"), names(x))
+        assert(!anyNaN(x$version))


I think this last one is wrong (should be anyNA()).

Generally, I'm not sure I like the messages produced by this package. There's of course a balance between offloading this stuff to well-tested functions and getting everything I want, but these seem too low-tech. For example:

library(checkmate) x <- 1:10 names(x) <- letters[1:10] assert_subset(c("geo_value", "time_value", "version"), names(x)) #> Error: Assertion on 'c("geo_value", "time_value", "version")' failed: Must be a subset of {'a','b','c','d','e','f','g','h','i','j'}, but has additional elements {'geo_value','time_value','version'}. assert_data_frame(x) #> Error: Assertion on 'x' failed: Must be of type 'data.frame', not 'integer'. version <- c(1:5, NA) assert(!anyNaN(version)) assert(!anyNA(version)) #> Error: Assertion on 'anyNA(version)' failed: FALSE.

The messages aren't wrapped or formatted as they were before. And the messages aren't nearly as clear. It's worth examining https://style.tidyverse.org/error-messages.html

should be anyNA()

Ah yea, in the idioms of this packages, it's anyMissing(). I can change that.

these seem too low-tech
the messages aren't nearly as clear

Seems like a matter of preference, so it's hard for me to know what you don't like about these. Custom error messages can always be done like this

if (!test_subset(c("geo_value", "time_value", "version"), names(x))) cli_abort("custom message")

Like you said, the off-loading to well tested functions is exactly what I like here. It's wild to me how much of our unit testing is the same "did this input type error correctly throw?" checks over and over. To me that's just downstream of how squirmy type checking can get in R. This package gives me type check tests I can trust, which frees up energy for testing the higher level logic of the package.

Maybe better then to refactor the arg_is_*() type functions as you did in epipredict to use {checkmate}. That way you mostly get better messaging.

In this particular case, to better mimic what the original messages were saying (though with better formatting), I'd like to see something like:

hardhat::validate_column_names(data.frame(geo_value = 1:5, time_value = 1:5), c("geo_value", "time_value", "version")) #> Error in `hardhat::validate_column_names()`: #> ! The following required columns are missing: 'version'.

^{Created on 2024-02-01 with reprex v2.0.2}

I've updated archive.R and other places where I do column checking with assert_subset to something like:

if (!test_subset(c("geo_value", "time_value", "version"), names(x))) cli_abort( "Columns `geo_value`, `time_value`, and `version` must be present in `x`." )

Sound good?

Are there other messages that you'd like changed? I'd prefer not to prematurely make wrappers (DRY three times).

dshemetov · 2024-02-07T00:56:39Z

R/archive.R

-            class = "epiprocess__version_values_must_not_be_na"
-          )
-        }
+        assert_data_frame(x)


The message from this function is similar:

r$> x = 5 r$> checkmate::assert_data_frame(x) Error: Assertion on 'x' failed: Must be of type 'data.frame', not 'double'.

dshemetov · 2024-02-07T00:58:43Z

R/archive.R

        }
+        assert_logical(compactify, len = 1, null.ok = TRUE)


The message from this function is similar:

r$> compactify = 5 r$> checkmate::assert_logical(compactify, len = 1, null.ok = TRUE) Error: Assertion on 'compactify' failed: Must be of type 'logical' (or 'NULL'), not 'double'.

dshemetov · 2024-02-07T01:01:00Z

R/archive.R

-        }
+        assert_set_equal(class(max_version), class(self$DT$version))
+        assert_set_equal(typeof(max_version), typeof(self$DT$version))
+        assert_scalar(max_version, na.ok = FALSE)


The messaging here is similar:

r$> max_version = c(5, 5) r$> checkmate::assert_scalar(max_version, na.o k = FALSE) Error: Assertion on 'max_version' failed: Must have length 1.

dshemetov · 2024-02-07T01:02:10Z

R/archive.R

        }
+        assert_logical(all_versions, len = 1)


r$> all_versions = c(1, 2) r$> assert_logical(all_versions, len = 1) Error: Assertion on 'all_versions' failed: Must be of type 'logical', not 'double'.

dshemetov · 2024-02-07T01:03:15Z

R/archive.R

-        }
+        assert_set_equal(class(max_version), class(self$DT$version))
+        assert_set_equal(typeof(max_version), typeof(self$DT$version))
+        assert_scalar(max_version, na.ok = FALSE)


The messaging here is similar:

r$> max_version = c(5, 5) r$> checkmate::assert_scalar(max_version, na.o k = FALSE) Error: Assertion on 'max_version' failed: Must have length 1.

dshemetov · 2024-02-07T01:03:59Z

R/correlation.R

@@ -78,12 +78,11 @@
 epi_cor <- function(x, var1, var2, dt1 = 0, dt2 = 0, shift_by = geo_value,
                    cor_by = geo_value, use = "na.or.complete",
                    method = c("pearson", "kendall", "spearman")) {
-  # Check we have an `epi_df` object
-  if (!inherits(x, "epi_df")) Abort("`x` must be of class `epi_df`.")
+  assert_class(x, "epi_df")


r$> assert_class(x, "epi_df") Error: Assertion on 'x' failed: Must inherit from class 'epi_df', but has class 'numeric'.

dshemetov · 2024-02-07T01:04:42Z

R/epi_df.R

-  if (!is.data.frame(x)) {
-    Abort("`x` must be a data frame.")
-  }
+  assert_data_frame(x)


r$> assert_data_frame(x) Error: Assertion on 'x' failed: Must be of type 'data.frame', not 'double'.

dshemetov · 2024-02-07T01:05:04Z

R/epi_df.R

-    Abort("`x` must be a data frame.")
-  }
+  assert_data_frame(x)
+  assert_list(additional_metadata)


r$> assert_list(additional_metadata)
Error: Assertion on 'additional_metadata' failed: Must be of type 'list', not 'double'.

dshemetov · 2024-02-07T01:05:52Z

R/grouped_epi_archive.R

-            epiprocess__DT_names = names(ungrouped$DT)
-          )
-        }
+        assert_class(ungrouped, "epi_archive")


r$> ungrouped = 5 r$> assert_class(ungrouped, "epi_archive") Error: Assertion on 'ungrouped' failed: Must inherit from class 'epi_archive', but has class 'numeric'.

dshemetov · 2024-02-07T01:06:12Z

R/grouped_epi_archive.R

-          )
-        }
+        assert_class(ungrouped, "epi_archive")
+        assert_character(vars)


r$> vars = 5 r$> assert_character(vars) Error: Assertion on 'vars' failed: Must be of type 'character', not 'double'.

dshemetov · 2024-02-07T01:07:23Z

R/grouped_epi_archive.R

        }
+        assert_logical(drop)


r$> assert_logical(5, len = 1) Error: Assertion on '5' failed: Must be of type 'logical', not 'double'.

I'm not trying to be an annoying blocker here. So feel free to ignore me. Here's the difference

Previously:

Now:

Do you feel like the previous one was clearer? Is it the rlang::last_trace() message that you wish was there?

dshemetov · 2024-02-07T01:13:35Z

R/grouped_epi_archive.R

        } else {
+          assert_numeric(ref_time_values, upper = private$ungrouped$versions_end, min.len = 1L, null.ok = FALSE, any.missing = FALSE)


r$> ref_time_values = Date() r$> assert_numeric(ref_time_values, upper = as.Date("2020-03-05"), min.len = 1 L, null.ok = FALSE, any.missing = FALSE) Error: Assertion on 'ref_time_values' failed: Must have length >= 1, but has length 0. r$> ref_time_values = c(as.Date("2020-03-06"), NA) r$> assert_numeric(ref_time_values, min.len = 1L, null.ok = FALSE, any.missing = FALSE) Error: Assertion on 'ref_time_values' failed: Contains missing values (element 2).

dshemetov · 2024-02-07T01:14:32Z

R/grouped_epi_archive.R

-        if (length(before) != 1L || is.na(before) || before < 0L) {
-          Abort("`before` must be length-1, non-NA, non-negative.")
-        }
+        assert_int(before, lower = 0L, null.ok = FALSE, na.ok = FALSE)


r$> before = c(1, 2) r$> assert_int(before, lower = 0L, null.ok = F ALSE, na.ok = FALSE) Error: Assertion on 'before' failed: Must have length 1. r$> before = NA r$> assert_int(before, lower = 0L, null.ok = F ALSE, na.ok = FALSE) Error: Assertion on 'before' failed: May not be NA. r$> before = -5 r$> assert_int(before, lower = 0L, null.ok = F ALSE, na.ok = FALSE) Error: Assertion on 'before' failed: Element 1 is not >= 0.

dshemetov · 2024-02-07T01:15:17Z

R/grouped_epi_archive.R

-        }
+        assert_logical(as_list_col, len = 1L)
+        assert_logical(all_versions, len = 1L)
+        assert_character(names_sep, len = 1L, null.ok = TRUE)


See examples for these above (messaging is similar).

dshemetov · 2024-02-07T01:16:18Z

R/grouped_epi_archive.R

-          if (!(is.atomic(comp_value) || is.data.frame(comp_value))) {
-            Abort("The slide computation must return an atomic vector or a data frame.")
-          }
+          assert(check_atomic(comp_value, any.missing = TRUE), check_data_frame(comp_value), combine = "or", .var.name = vname(comp_value))


r$> comp_value = list(c(2)) r$> assert(check_atomic(comp_value, any.missing = TRUE), check_data_frame(comp _value), combine = "or", .var.name = vname (comp_value)) Error: Assertion on 'comp_value' failed: One of the following must apply: * check_atomic(comp_value): Must be of * type 'atomic', not 'list' * check_data_frame(comp_value): Must be * of type 'data.frame', not 'list'.

dshemetov · 2024-02-07T01:17:08Z

R/methods-epi_archive.R

@@ -77,7 +77,7 @@
 #'
 #' @export
 epix_as_of <- function(x, max_version, min_time_value = -Inf, all_versions = FALSE) {
-  if (!inherits(x, "epi_archive")) Abort("`x` must be of class `epi_archive`.")
+  assert_class(x, "epi_archive")


For all these class messages, the default messaging is similar, see above.

dshemetov · 2024-02-07T01:18:25Z

R/slide.R

+    )
+    if (anyDuplicated(ref_time_values) != 0L) {
+      cli_abort("`ref_time_values` must not contain any duplicates; use `unique` if appropriate.")
+    }


See similar section in grouped_epi_archive above.

dshemetov · 2024-02-07T01:18:48Z

R/slide.R

-    if (length(before) != 1L || is.na(before) || before < 0L) {
-      Abort("`before` must be length-1, non-NA, non-negative")
-    }
+    assert_int(before, lower = 0, null.ok = FALSE, na.ok = FALSE)


Default messages are similar, see above.

dshemetov · 2024-02-07T01:20:35Z

@dajmcdon I've updated this PR with better messaging in certain cases. I also went through all the instances where I changed to using a checkmate assert and added an example of its error message. I think the remaining ones are all very similar to what we had before. Let me know what you think!

dajmcdon

I added one comment here: #413 (comment).

If you're fine with it then so am I.

dshemetov · 2024-02-07T01:41:22Z

Thanks! I'm happy to polish a little more. Just let me know what you'd prefer to see that in that case.

* swap rlang for cli in Abort and Warn * remove Abort and Warn plugins * format long error message lines * replace and remove break_str * simplify validate_version_bound with checkmate * use checkmate for validation where possible * update tests

dshemetov · 2024-02-07T19:20:28Z

Going to merge for now, but happy to polish messages in another PR.

dshemetov requested a review from dajmcdon February 1, 2024 01:45

dshemetov force-pushed the ds/error-msg branch 2 times, most recently from cc30c3a to de91c22 Compare February 1, 2024 02:01

dajmcdon reviewed Feb 1, 2024

View reviewed changes

dshemetov commented Feb 7, 2024

View reviewed changes

dshemetov requested a review from dajmcdon February 7, 2024 01:19

dajmcdon approved these changes Feb 7, 2024

View reviewed changes

dshemetov force-pushed the ds/error-msg branch 3 times, most recently from 2d91421 to ea7872b Compare February 7, 2024 03:32

refactor: error messages

c897c69

* swap rlang for cli in Abort and Warn * remove Abort and Warn plugins * format long error message lines * replace and remove break_str * simplify validate_version_bound with checkmate * use checkmate for validation where possible * update tests

doc+build: news and version bump

d46aa2f

dshemetov force-pushed the ds/error-msg branch from ea7872b to d46aa2f Compare February 7, 2024 03:34

dshemetov merged commit aecb7e5 into dev Feb 7, 2024

dshemetov deleted the ds/error-msg branch February 7, 2024 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: clean up error messages #413

refactor: clean up error messages #413

dshemetov commented Feb 1, 2024 •

edited

Loading

dajmcdon Feb 1, 2024

dshemetov Feb 1, 2024 •

edited

Loading

dajmcdon Feb 1, 2024

dshemetov Feb 1, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dajmcdon Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024 •

edited

Loading

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024 •

edited

Loading

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov Feb 7, 2024

dshemetov commented Feb 7, 2024

dajmcdon left a comment

dshemetov commented Feb 7, 2024

dshemetov commented Feb 7, 2024

		} else {
		assert_numeric(ref_time_values, upper = private$ungrouped$versions_end, min.len = 1L, null.ok = FALSE, any.missing = FALSE)

refactor: clean up error messages #413

refactor: clean up error messages #413

Conversation

dshemetov commented Feb 1, 2024 • edited Loading

Checklist

Change explanations for reviewer

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

Choose a reason for hiding this comment

dshemetov Feb 1, 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

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

dshemetov Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshemetov Feb 7, 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

dshemetov commented Feb 7, 2024

dajmcdon left a comment

Choose a reason for hiding this comment

dshemetov commented Feb 7, 2024

dshemetov commented Feb 7, 2024

dshemetov commented Feb 1, 2024 •

edited

Loading

dshemetov Feb 1, 2024 •

edited

Loading

dshemetov Feb 7, 2024 •

edited

Loading

dshemetov Feb 7, 2024 •

edited

Loading