-
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
Conversation
| ret <- data.frame(conc = conc, time = time) | ||
| mask_start <- time %in% start | ||
| if (!any(mask_start)) { | ||
| all_concs <- conc[time >= start & time <= end] |
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 filter all_concs, all_times, and your is.na() test below can be on c1. That will also ensure that c1 is 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 any time=NA all_concs will contain NA, let me know what you think of it!
| data.frame(conc = c(3, 3, 1), time = 1:3), | ||
| ignore_attr = TRUE | ||
| ) | ||
| }) |
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 add tests for:
- All concentrations are NA
- All times are NA
- All concentrations are 0
(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.)
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.
Hey sorry for the delay on this @billdenney! Some questions regarding the cases and their expected outputs:
- All concentrations are NA
Initially I developed the functions using the same strucuture as PKNCA_impute_method_start_cmin:
PKNCA_impute_method_start_cmin <- function(conc, time, start, end, ..., options = list()) {
ret <- data.frame(conc = conc, time = time)
mask_start <- time %in% start
if (!any(mask_start)) {
all_concs <- conc[start <= time & time <= end]
if (!all(is.na(all_concs))) {
cmin <- min(all_concs, na.rm = TRUE)
ret <- rbind(ret, data.frame(time = start, conc = cmin))
ret <- ret[order(ret$time), ]
}
}
ret
}
In this case, all conc = NA will return the same dataset for PKNCA_impute_method_start_cmin
But if for PKNCA_impute_method_start_c1 we want directly the original pk.calc.c0.method.c1 handling the NA case like in the original function, when all conc = NA then start = NA with a warning message
Is this the desired behavior we would like to have for PKNCA_impute_method_start_c1 even if it is different to PKNCA_impute_method_start_cmin? The same with PKNCA_impute_method_start_logslope?
- All times are NA
Returns same dataset for PKNCA_impute_method_start_cmin
Returns error for PKNCA_impute_method_start_c1 with default pk.calc.c0.method.c1 behaviour
- All concs are 0
Should PKNCA_impute_method_start_logslope return same dataset or NA?
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 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
-
All concentrations are NA:
- The function does not change the input data. The output will have the same NA concentrations as the input.
-
All times are NA:
- The function does not change the input data. The output will have the same NA times as the input.
-
All concentrations are 0:
- The function does not change the input data. The output will have the same 0 concentrations as the input.
PKNCA_impute_method_start_c1
-
All concentrations are NA:
- The function adds the
starttime with an NA concentration to the data. The output will have NA concentrations, including the imputed start time.
- The function adds the
-
All times are NA:
- The function raises an error indicating that the
timevector contains missing values.
- The function raises an error indicating that the
-
All concentrations are 0:
- The function adds the
starttime with a 0 concentration to the data. The output will have 0 concentrations, including the imputed start time.
- The function adds the
|
In order to keep working in other issues I will reframe the branch strategy in my fork. Unfortunately, I have no permissions to simply modify the development branch on this pr, so I duplicated it with the correct branch (#383) and close this one. |
New PKNCA_method_start functions that could perhaps be default/standard. Feel free to give them a look and merge them if they are useful!
Closes #350