-
Notifications
You must be signed in to change notification settings - Fork 93
Remove redundant test on trunc_probs()
#970
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
these aspects are already covered by the other test
probs_2 <- probs_1 | ||
probs_2[3] <- NA_real_ | ||
|
||
expect_equal(parsnip:::trunc_probs(probs_1, 0), probs_1) |
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.
this covers "leave probs untouched" and "check the full vector", both of which are covered by
parsnip/tests/testthat/test-survival-censoring-weights.R
Lines 18 to 22 in ab42409
probs <- (1:200)/200 | |
expect_identical( | |
parsnip:::trunc_probs(probs, trunc = 0.01), | |
probs | |
) |
probs_2[3] <- NA_real_ | ||
|
||
expect_equal(parsnip:::trunc_probs(probs_1, 0), probs_1) | ||
expect_equal(parsnip:::trunc_probs(probs_2, 0), probs_2) |
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.
this one adds an NA
which is also part of the following, the whole vector is just covered over multiple expectations
parsnip/tests/testthat/test-survival-censoring-weights.R
Lines 13 to 16 in ab42409
probs_trunc_04_na <- parsnip:::trunc_probs(c(NA, probs), 0.4) | |
expect_identical(probs_trunc_04_na[1], NA_real_) | |
expect_equal(probs_trunc_04_na[2], data_derived_trunc) | |
expect_equal(probs_trunc_04_na[3:6], probs[2:5]) |
expect_equal( | ||
parsnip:::trunc_probs(probs_1, 0.1), | ||
ifelse(probs_1 < 0.05 / 2, 0.05 / 2, probs_1) | ||
) |
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.
this checks for what I called "data derived truncation" in
parsnip/tests/testthat/test-survival-censoring-weights.R
Lines 8 to 11 in ab42409
probs_trunc_04 <- parsnip:::trunc_probs(probs, trunc = 0.4) | |
data_derived_trunc <- min(probs[probs > 0]) / 2 | |
expect_equal(probs_trunc_04[1], data_derived_trunc) | |
expect_equal(probs_trunc_04[2:5], probs[2:5]) |
expect_equal(min(parsnip:::trunc_probs(probs_2, 0.1), na.rm = TRUE), 0.05 / 2) | ||
expect_equal(is.na(parsnip:::trunc_probs(probs_2, 0.1)),is.na(probs_2)) | ||
}) |
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.
This one checks that there is an NA in the right place and the data-derived truncation value kicks in. This is also checked in
parsnip/tests/testthat/test-survival-censoring-weights.R
Lines 13 to 16 in ab42409
probs_trunc_04_na <- parsnip:::trunc_probs(c(NA, probs), 0.4) | |
expect_identical(probs_trunc_04_na[1], NA_real_) | |
expect_equal(probs_trunc_04_na[2], data_derived_trunc) | |
expect_equal(probs_trunc_04_na[3:6], probs[2:5]) |
This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue. |
I think the test removed in this PR is mainly to test aspects which are already covered by the other test on
trunc_probs()
. I'll comment with links :)