-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
Tiny improvments for ResamplePredition and an important question #1324
Conversation
tenull = sapply(preds.test, is.null) | ||
trnull = sapply(preds.train, is.null) | ||
tenull = vapply(preds.test, is.null, FUN.VALUE = logical(1)) | ||
trnull = vapply(preds.train, is.null, FUN.VALUE = logical(1)) | ||
if (any(tenull)) pr.te = preds.test[!tenull] else pr.te = preds.test | ||
if (any(trnull)) pr.tr = preds.train[!trnull] else pr.tr = preds.train |
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.
If we assume that preds.test
and preds.train
never contains 'NULL' (as we discussed in our weekly meeting), we could even remove lines 19 - 22. If not, I think we can at least prettify the code and replace lines 19 - 22 with pr.te = filterNull(preds.test)
and pr.tr = filterNull(preds.train)
.
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.
@MariaErdmann what is the status here? Is filterNull
appropriate here?
@@ -16,8 +16,8 @@ NULL | |||
|
|||
|
|||
makeResamplePrediction = function(instance, preds.test, preds.train) { | |||
tenull = sapply(preds.test, is.null) | |||
trnull = sapply(preds.train, is.null) | |||
tenull = vapply(preds.test, is.null, FUN.VALUE = logical(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.
use vlapply
tenull = sapply(preds.test, is.null) | ||
trnull = sapply(preds.train, is.null) | ||
tenull = vapply(preds.test, is.null, FUN.VALUE = logical(1)) | ||
trnull = vapply(preds.train, is.null, FUN.VALUE = logical(1)) | ||
if (any(tenull)) pr.te = preds.test[!tenull] else pr.te = preds.test | ||
if (any(trnull)) pr.tr = preds.train[!trnull] else pr.tr = preds.train |
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.
@MariaErdmann what is the status here? Is filterNull
appropriate here?
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.
See changes requested by Giuseppe.
no. and iteration will either work, and produce a normal prediction object, or fail with an exception. so if you dont see further reasons in the code why a NULL could be produced, i am very sure that is impossible. |
@MariaErdmann what's the status here? |
cf3db65
to
57f099e
Compare
I guess this PR won't be finished? |
Prio seems to be low and improvements tiny. Closing. Feel free to re-open if you want to finish it :) |
After discussing issue #1284 and PR #1315 respectively on our last meeting, I did some beauty to ResamplePrediction:
There is one major question that arose regarding the ResamplePrediction function and
depending on the answer some probably more changes need to be implemented.
Is it possible to make resampling, i.e. cross-validation, where the result of one of the iterations
is NULL? To demonstrate what we mean you find a small code snippet below:
Is there any use case you can think about where this might happen?
Thank you!