Test growth_rate #197

kenmawer · 2022-08-10T23:37:42Z

Note that I was unable to prompt this warning message:

Warn("xcontains duplicate values. (If being run on a column in anepi_df, did you group by relevant key variables?)")

…nd y as c(1,1:19), for example, won't work.

brookslogan · 2022-08-11T00:13:44Z

Think you can trigger that warning + some error with

jhu_csse_daily_subset %>% 
  # group_by(geo_value) %>% 
  mutate(cases_gr = growth_rate(x = time_value,  y = cases, dup_rm=TRUE))

but without dup_rm=TRUE, it actually returns a result.... is this a bug? Do you know what the result would represent in this case?

tests/testthat/test-growth_rate.R

brookslogan · 2022-08-11T00:26:53Z

tests/testthat/test-growth_rate.R

+  expect_error(growth_rate(x=1:20,y=1:20,x0=21),
+               "`x0` must be a subset of `x`.")
+  expect_error(growth_rate(x=1:20,y=1:20,x0=c(1,3)),NA)
+})


Could you please add another test for the actual values coming out? E.g., construct a simple example where the growth rate should be obvious, and test that we calculate that obvious value. At least for some of the methods.

Also, please test that each of the methods, log_scale settings, and na_rm settings at least run successfully.

If you can't construct an example where you think you know what the output should be, maybe throw in some testthat::expect_snapshots? I assume we'll need to take the expect_snapshot approach for some/most/all of the methods; I'm not sure, e.g., that there's a simple example where we would think the trendfilter output is obvious.

expect_snapshot creates a new file for snapshots and does something that doesn't seem inherently helpful. All it does it checks for errors from my experience; however, that doesn't seem to be any better than doing expect_error.

@kenmawer I think the point here is to test that the correct calculations are occurring when arguments are correctly specified, not just that errors happen when the inputs are wrong.

I already added a few tests for calculations. I would suspect that for some of the other methods, it would be too complicated to be able to verify the outputted numbers. It also depends on what you mean by "correct calculations," whether it's merely getting the right number of cells or getting those exact numbers.

Since it's likely too complicated to verify the output numbers [for the complex settings like using trend filtering], that's why I suggested expect_snapshot. When you first run it, it saves the current output to a _snaps directory, which you would then commit (make sure that the test is constructed so that this file is very small; we don't want to bloat the repo), and then later, when we run the snapshot test again, it will detect the file in the _snaps directory and compare against it. If we inadvertently change the output behavior, this test will catch it.

Looks like of the requested

test that each of the methods, log_scale settings, and na_rm settings at least run successfully

only the method outputs are left -- is that correct?

…nished). Also noted that `rel_change` is the default method for estimation.

…ailed to properly eliminate NA's.

…ppen...

… hand editing. Merge branch 'main' of https://github.com/cmu-delphi/epiprocess into km-test-growth_rate # Conflicts: # man/epi_archive.Rd # man/epix_merge.Rd

R/growth_rate.R

dajmcdon · 2022-08-17T16:32:11Z

tests/testthat/test-growth_rate.R

+  expect_error(growth_rate(x=1:20,y=1:20,x0=21),
+               "`x0` must be a subset of `x`.")
+  expect_error(growth_rate(x=1:20,y=1:20,x0=c(1,3)),NA)
+})


@kenmawer I think the point here is to test that the correct calculations are occurring when arguments are correctly specified, not just that errors happen when the inputs are wrong.

nmdefries

Some questions about desired behavior and some about the state of previous review comments.

nmdefries · 2023-06-08T20:32:30Z

R/growth_rate.R

@@ -145,7 +146,7 @@ growth_rate = function(x = seq_along(y), y, x0 = x,

  # Remove NAs if we need to
  if (na_rm) {
-    o = !(is.na(x) & is.na(y))
+    o = !is.na(x) & !is.na(y)


question: Is this a bug fix? The new version of this line doesn't mean the same thing as the old version.

suggestion: I'm assuming the new version is desired behavior (keeping only pairs of obs in x and y when both values are available), but the comment "# Remove NAs if we need to" is very ambiguous. Recommend making the goal more explicit.

tests/testthat/test-growth_rate.R

nmdefries · 2023-06-20T21:47:10Z

tests/testthat/test-growth_rate.R

+test_that("Test error throwing",{
+  # Error cases
+  expect_error(growth_rate(x=1:3,y=1:4),
+               "`x` and `y` must have the same length.")


suggestion [non-blocking]: Probably beyond scope here, but it would be nice if the errors and warnings in growth_rate had classes so we could check for that rather than message wording, which is liable to change.

nmdefries · 2023-06-20T21:50:14Z

tests/testthat/test-growth_rate.R

+    expect_warning() %>%
+    expect_error()


issue: It's hard to tell what this is doing. If we want to check for both a warning and an error, run this twice with each expect_*. If we're trying to do something else, I don't understand.

nmdefries · 2023-06-20T21:56:12Z

tests/testthat/test-growth_rate.R

+})
+
+test_that("Running different methods with NA removal won't fail",{
+  for (m in methods) expect_false(NA %in% gr(method = m,x0=1:5)) 


question: x0 doesn't contain any NAs, so are we really testing NA-removal? Do we want

Suggested change

for (m in methods) expect_false(NA %in% gr(method = m,x0=1:5))

for (m in methods) expect_false(NA %in% gr(method = m,x0=c(1:5, NA)))

?

nmdefries · 2023-06-20T21:57:56Z

tests/testthat/test-growth_rate.R

+  expect_error(growth_rate(x=1:20,y=1:20,x0=21),
+               "`x0` must be a subset of `x`.")
+  expect_error(growth_rate(x=1:20,y=1:20,x0=c(1,3)),NA)
+})


Looks like of the requested

test that each of the methods, log_scale settings, and na_rm settings at least run successfully

only the method outputs are left -- is that correct?

dajmcdon · 2023-06-20T22:21:36Z

@nmdefries I'm not sure whether we shouldn't just close this at this point. Maybe investigate if the possible bug fix is needed, but close without merging?

nmdefries · 2023-06-20T22:24:40Z

@dajmcdon Thanks for the feedback. I was tagged recently so not sure what the context is here.

@brookslogan Thoughts?

kenmawer added 4 commits July 21, 2022 18:02

Added a few more tests.

6306558

Renamed file.

6b9ae45

I can't seem to figure out how to get the warning thrown. Setting x a…

4ce0fcd

…nd y as c(1,1:19), for example, won't work.

Added newline.

741ecd2

kenmawer requested review from dajmcdon and brookslogan as code owners August 10, 2022 23:37

brookslogan reviewed Aug 11, 2022

View reviewed changes

tests/testthat/test-growth_rate.R Outdated Show resolved Hide resolved

brookslogan reviewed Aug 11, 2022

View reviewed changes

tests/testthat/test-growth_rate.R Show resolved Hide resolved

brookslogan reviewed Aug 11, 2022

View reviewed changes

kenmawer added 5 commits August 10, 2022 18:42

Added another test.

58cafd8

Addded updated documentation, as well as tests on methods (still unfi…

8b82d9d

…nished). Also noted that `rel_change` is the default method for estimation.

Added more tests.

6850a9a

Fixed na_rm method in growth_rate that I believed was wrong as it f…

46690de

…ailed to properly eliminate NA's.

Added another test to see what happens when NA's are removed.

fda2e1a

kenmawer changed the title ~~Test growth-rate~~ Test growth_rate Aug 11, 2022

kenmawer added 2 commits August 11, 2022 14:38

Doing snapshot creates a new snap file, which I just don't want to ha…

2556d55

…ppen...

Resolved a merge conflict on Rd's that I don't want to tamper with by…

eab1d78

… hand editing. Merge branch 'main' of https://github.com/cmu-delphi/epiprocess into km-test-growth_rate # Conflicts: # man/epi_archive.Rd # man/epix_merge.Rd

kenmawer requested a review from brookslogan August 11, 2022 21:51

dajmcdon requested changes Aug 17, 2022

View reviewed changes

kenmawer added 5 commits August 17, 2022 11:06

Made changes.

7141537

Added another example to test for na_rm.

1c26f37

Good commit

b65ab18

Added growth_rate test.

c973778

Condensed a for loop.

bc04213

brookslogan mentioned this pull request Mar 3, 2023

Implement testing across the package #42

Closed

brookslogan requested a review from nmdefries May 24, 2023 19:37

nmdefries requested changes Jun 20, 2023

View reviewed changes

dajmcdon closed this Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test growth_rate #197

Test growth_rate #197

kenmawer commented Aug 10, 2022 •

edited

Loading

brookslogan commented Aug 11, 2022 •

edited

Loading

brookslogan Aug 11, 2022 •

edited

Loading

brookslogan Aug 11, 2022 •

edited

Loading

kenmawer Aug 11, 2022

dajmcdon Aug 17, 2022

kenmawer Aug 17, 2022

brookslogan Aug 17, 2022 •

edited

Loading

nmdefries Jun 20, 2023

dajmcdon Aug 17, 2022

nmdefries left a comment

nmdefries Jun 8, 2023

nmdefries Jun 20, 2023

nmdefries Jun 20, 2023

nmdefries Jun 20, 2023

nmdefries Jun 20, 2023

dajmcdon commented Jun 20, 2023

nmdefries commented Jun 20, 2023

	for (m in methods) expect_false(NA %in% gr(method = m,x0=1:5))
	for (m in methods) expect_false(NA %in% gr(method = m,x0=c(1:5, NA)))

Test growth_rate #197

Test growth_rate #197

Conversation

kenmawer commented Aug 10, 2022 • edited Loading

brookslogan commented Aug 11, 2022 • edited Loading

brookslogan Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

brookslogan Aug 11, 2022 • 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

brookslogan Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmdefries 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

dajmcdon commented Jun 20, 2023

nmdefries commented Jun 20, 2023

kenmawer commented Aug 10, 2022 •

edited

Loading

brookslogan commented Aug 11, 2022 •

edited

Loading

brookslogan Aug 11, 2022 •

edited

Loading

brookslogan Aug 11, 2022 •

edited

Loading

brookslogan Aug 17, 2022 •

edited

Loading