-
Notifications
You must be signed in to change notification settings - Fork 29
Feature request: New logslope & c1 impute_start methods #350 #351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
273d3b4
d8c7519
9cc0ffb
6b4b435
4b3d6cc
d65ad9f
32fe6fd
7d215f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,6 +92,90 @@ test_that("PKNCA_impute_method_start_cmin", { | |
|
|
||
| }) | ||
|
|
||
| test_that("PKNCA_impute_method_start_logslope", { | ||
| # No imputation when start is in the data | ||
| expect_equal( | ||
| PKNCA_impute_method_start_logslope(conc = 3:1, time = 0:2, start = 0, end = 2), | ||
| data.frame(conc = 3:1, time = 0:2) | ||
| ) | ||
| # Impute when start is not in the data | ||
| expect_equal( | ||
| PKNCA_impute_method_start_logslope(conc = 3:1, time = 1:3, start = 0, end = 3), | ||
| data.frame(conc = c(4.5, 3:1), time = 0:3), | ||
| ignore_attr = TRUE | ||
| ) | ||
| # Data outside the interval are ignored (before interval) | ||
| expect_equal( | ||
| PKNCA_impute_method_start_logslope(conc = c(0, 2:1), time = c(-1, 1:2), start = 0, end = 2), | ||
| data.frame(conc = c(0, 4, 2:1), time = c(-1, 0, 1:2)), | ||
| ignore_attr = TRUE | ||
| ) | ||
| # No modification if no C1 -> C2 decline in samples | ||
| expect_equal( | ||
| PKNCA_impute_method_start_logslope(conc = c(1, 1, 1), time = 1:3, start = 0, end = 3), | ||
| data.frame(conc = c(1, 1, 1), time = 1:3), | ||
| ignore_attr = TRUE | ||
| ) | ||
| # No modification if C1 = C2 in samples | ||
| expect_equal( | ||
| PKNCA_impute_method_start_logslope(conc = c(3, 3, 1), time = 1:3, start = 0, end = 3), | ||
| data.frame(conc = c(3, 3, 1), time = 1:3), | ||
| ignore_attr = TRUE | ||
| ) | ||
| # All concentrations are NA -> does not change | ||
| expect_equal( | ||
| PKNCA_impute_method_start_logslope(conc = c(NA, NA, NA), time = 1:3, start = 0, end = 3), | ||
| data.frame(conc = c(NA, NA, NA), time = 1:3) | ||
| ) | ||
| # All times are NA -> does not change | ||
| expect_equal( | ||
| PKNCA_impute_method_start_logslope(conc = 1:3, time = c(NA, NA, NA), start = 0, end = 3), | ||
| data.frame(conc = 1:3, time = c(NA, NA, NA)) | ||
| ) | ||
| # All concentrations are 0 -> does not change | ||
| expect_equal( | ||
| PKNCA_impute_method_start_logslope(conc = c(0, 0, 0), time = 1:3, start = 0, end = 3), | ||
| data.frame(conc = c(0, 0, 0), time = 1:3) | ||
| ) | ||
| }) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add tests for:
(I see that list of tests is needed for the other imputation methods, too. I just added an issue, #361, to add those after this PR is merged.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey sorry for the delay on this @billdenney! Some questions regarding the cases and their expected outputs:
Initially I developed the functions using the same strucuture as In this case, all But if for Is this the desired behavior we would like to have for
Returns same dataset for
Should
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept their original behaviours of these testing cases as on my initial pr. Let me know if you think I need to standardize them. This is probably something we need to consider on the other PKNCA_impute_method_start when doing #361 PKNCA_impute_method_start_logslope
PKNCA_impute_method_start_c1
|
||
|
|
||
| test_that("PKNCA_impute_method_start_c1", { | ||
| # No imputation when start is in the data | ||
| expect_equal( | ||
| PKNCA_impute_method_start_c1(conc = 1:3, time = 0:2, start = 0, end = 2), | ||
| data.frame(conc = 1:3, time = 0:2) | ||
| ) | ||
| # Impute when start is not in the data | ||
| expect_equal( | ||
| PKNCA_impute_method_start_c1(conc = 1:3, time = 1:3, start = 0, end = 3), | ||
| data.frame(conc = c(1, 1:3), time = 0:3), | ||
| ignore_attr = TRUE | ||
| ) | ||
| # Data outside the interval are ignored (before interval) | ||
| expect_equal( | ||
| PKNCA_impute_method_start_c1(conc = 1:3, time = c(-1, 1:2), start = 0, end = 2), | ||
| data.frame(conc = c(1, 2, 2:3), time = c(-1, 0, 1:2)), | ||
| ignore_attr = TRUE | ||
| ) | ||
| # All concentrations are NA | ||
| expect_equal( | ||
| PKNCA_impute_method_start_c1(conc = c(NA, NA, NA), time = 1:3, start = 0, end = 3), | ||
| data.frame(conc = c(NA, NA, NA, NA), time = 0:3), | ||
| ignore_attr = TRUE | ||
| ) | ||
| # All times are NA | ||
| expect_error( | ||
| PKNCA_impute_method_start_c1(conc = 1:3, time = c(NA, NA, NA), start = 0, end = 3), | ||
| "Assertion on 'time' failed: Contains missing values" | ||
| ) | ||
| # All concentrations are 0 | ||
| expect_equal( | ||
| PKNCA_impute_method_start_c1(conc = c(0, 0, 0), time = 1:3, start = 0, end = 3), | ||
| data.frame(conc = c(0, 0, 0, 0), time = 0:3), | ||
| ignore_attr = TRUE | ||
| ) | ||
| }) | ||
|
|
||
| test_that("PKNCA_impute_fun_list", { | ||
| expect_equal( | ||
| PKNCA_impute_fun_list(NA_character_), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use
pk.calc.c0.method.c1. That way, you don't need to filterall_concs,all_times, and youris.na()test below can be onc1. That will also ensure thatc1is consistently calculated anywhere it is used within PKNCA.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an assert before
all_concs <-because if anytime=NAall_concs will contain NA, let me know what you think of it!