introduce Aggregation class properties and clean up ResampleResult printer #1187
Conversation
|
this is more or less a core change. so please review very carefully! |
| a = measure$aggr | ||
| p = a$properties | ||
| pred = rdesc$predict | ||
| p.allowed = if (all(c("req.train", "req.test") %in% p)) { |
There was a problem hiding this comment.
Why not setequal here?
There was a problem hiding this comment.
Why not setequal here?
because my code expresses what i want to say, and is more robust than setequal. consider that the aggregation properties might grow larger. and contain other stuff.
|
Looks good to me, but somebody else should review as well. |
thx + agree |
|
@jakob-r @mllg @zmjones @schiffner |
|
looks good to me. is there a reason for the expect_false -> is.na setup? could this ever be NaN or something other than a length 1 numeric? |
do you mean the code cleanup i did test_base_resample? resampleresult$aggr can be NA when the training or something else failed. |
|
yes that is what i meant. then merge away! i fixed a grammatical error in the docs but that is it. |
|
thx. someone pls merge when travis is done |
|
should i wait for jenkins? |
this addresses issue #1045
it became a lot longer than i had hoped.
summary of what i did:
Aggregation objects now have properties. they define whether they need the trains set evals, test set evals, or both. this allows to generate an informative error msg for the NA problem in the issue.
i removed the "mean" and "sd" display from the ResampleResult printer. this was kinda hacked in before, confusing as it used different aggregation functions, and also did not reliably work.
it also nearly never provided more info but ate up extra lines in a useless manner.
i printed the aggreated measure performance in a standard way instead.
some tests