-
Notifications
You must be signed in to change notification settings - Fork 10
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
Rxr 421 - Unit tests for 2-cmt pk functions #13
Conversation
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.
Looks good!
|
||
switch( | ||
phase, | ||
"both" = list(alpha = log(2)/alpha, beta = log(2)/beta), |
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.
Could we either return obj
here or remove the where it's defined above?
tests/testthat/test_pk_2cmt_bolus.R
Outdated
expect_true(all(c("t", "dv") %in% colnames(res))) | ||
expect_true(inherits(res, "data.frame")) |
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.
These are fine expect_named(res, c("t", "dv"))
and expect_s3_class(res, "data.frame")
would also work and might be a little clearer.
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.
yeah good idea! I forget about some of the more specific ones.
Was waiting for some things to run so I added a couple unit tests. Along the way I did a bit of clean-up. One functional change: our bolus infusion equations run into some continuity problems as delta_t approaches 0, which made the pk_2cmt_t12_interval() function return -Inf when that didn't make sense, so I added a very small offset, which produces the desired number (as far as I can tell).
Where possible, and as noted in the tests, I verified all the results with PKPDsim.