add more asserts to task operator #1474

berndbischl · 2017-02-02T19:33:23Z

fixes #1467

berndbischl · 2017-02-02T19:38:11Z

please make commit msg better when you squash, srewed this up
this is a base change read CAREFULLY

berndbischl · 2017-02-02T19:51:14Z

rebased on master for travis fix from @jakob-r

larskotthoff · 2017-02-02T22:26:43Z

Why is everything in checkTaskOrDesc commented out?

berndbischl · 2017-02-02T22:28:20Z

Why is everything in checkTaskOrDesc commented out?

uuh. mistake. fixing

berndbischl · 2017-02-02T23:00:16Z

please donte merge, this is harder than expected

berndbischl · 2017-02-02T23:25:28Z

ok, new try @larskotthoff

i had to touch way too much, but opted to make the structure better.
i changed NEWS already

read carefully

larskotthoff · 2017-02-02T23:46:09Z

R/Task_operators.R

 #'
 #' @template arg_task_or_desc
-#' @return [\code{\link{TaskDesc}}].
+#' @return ret_taskdesc


Should be @template ret_taskdesc?

larskotthoff · 2017-02-02T23:47:05Z

R/getFeatureImportance.R

  makeS3Obj("FeatureImportance",
    res = imp,
-    task.desc = getTaskDescription(object),
+    task.desc = object$task.desc,


Why not use the getter anymore here?

berndbischl · 2017-02-03T00:38:22Z

task.desc = getTaskDescription(object)
vs
task.desc = object$task.desc

Why not use the getter anymore here?

the reason that i cannot is basically the larger change i did here.

before my PR getTaskDesc had an undefined API. which class was i allowed to pass in?
(docs were wrong and arg checks did not exist)
it was also against our policy to have s3 methods that "wildly dispatch across many different classes"
getters are supposed to be called getClassSlot.

now in that piece of code "object" is a WrappedModel.
If you want me to complete the PR in a better way I would need to add the getter
getWModelTaskDescription

I can do that. Was simply a bit lazy and needed to catch the night bus

larskotthoff · 2017-02-03T00:40:33Z

If our policy is still to add getters for everything rather than doing it manually, then yes, you should add the getter :)

mllg · 2017-02-14T09:33:55Z

R/Task_operators.R

-#' @export
-getTaskDescription.TaskDesc = function(x) {
-  x
+  checkTask(x, allow.desc = TRUE)


You are dodging S3 here? Should be getTaskDescription.Task() and getTaskDescription.TaskDescription().

mllg · 2017-02-14T09:35:33Z

R/Task_operators.R

-
-#' @export
-getTaskClassLevels.TaskDescClassif = function(x) {
+  checkTask(x, allow.desc = TRUE, task.type = c("classif", "multilabel"))


getTaskClassLevels.ClassifTask() + getTaskClassLevels.MultilabelTask()?

mllg · 2017-02-14T09:36:10Z

R/Task_operators.R

 #' Target column name is not included.
 #'
 #' @template arg_task
 #' @return [\code{character}].
 #' @family task
 #' @export
 getTaskFeatureNames = function(task) {
+  assertClass(task, "Task")


getTaskFeatureNames.SupervisedTask()?

mllg · 2017-02-14T09:38:44Z

R/Task_operators.R

@@ -252,6 +253,8 @@ getTaskTargets.CostSensTask = function(task, recode.target = "no") {
 #' head(getTaskData(task, subset = 1:100, recode.target = "01"))
 getTaskData = function(task, subset, features, target.extra = FALSE, recode.target = "no") {


Could also be getTaskData.Task()

mllg · 2017-02-14T09:39:11Z

R/Task_operators.R

@@ -380,6 +386,7 @@ recodeSurvivalTimes = function(y, from, to) {
 #' @family task
 #' @export
 getTaskCosts = function(task, subset) {
+  assertClass(task, "Task")
  if (task$task.desc$type != "costsens")


getTaskCosts.CostSensTask()

mllg · 2017-03-19T19:23:05Z

tried to merge master, many conflicts ... :(

berndbischl · 2017-03-19T19:27:21Z

tried to merge master, many conflicts ... :(

shit. where are we now? in what state? shall i roll something back? i want to have this in now, otherwise all work is lost...

berndbischl · 2017-03-19T19:29:08Z

i should really not have followed @jakob-r request for renaming. this is the result now. we have to do this more carefully!

mllg · 2017-03-19T20:01:20Z

Continued in other PR.

* re-implemented #1474 * add default for getTaskDesc * missing NAMESPACE * fix for plotViperCharts * fix costsens bug * getTaskCosts only for Tasks

berndbischl mentioned this pull request Feb 2, 2017

getTaskFormula should check if x is actually a task object #1467

Closed

berndbischl added pr-please review prio-high pr-ready for merge (?) labels Feb 2, 2017

berndbischl requested a review from larskotthoff February 2, 2017 19:41

...

9e54d19

berndbischl force-pushed the add_asserts_to_task_operators branch from c132415 to 9e54d19 Compare February 2, 2017 19:50

berndbischl added pr-work in progress - not done and removed pr-ready for merge (?) labels Feb 2, 2017

...

f6270c9

berndbischl removed the pr-work in progress - not done label Feb 2, 2017

larskotthoff requested changes Feb 2, 2017

View reviewed changes

mllg requested changes Feb 14, 2017

View reviewed changes

merged conflict

68ca8df

mllg self-assigned this Mar 8, 2017

use s3

f378798

Coorsaa added the project - base label Mar 9, 2017

merged master

56030f7

mllg added a commit that referenced this pull request Mar 19, 2017

re-implemented #1474

207f7a1

mllg closed this Mar 19, 2017

mllg mentioned this pull request Mar 19, 2017

Use more S3 in task ops #1711

Merged

larskotthoff pushed a commit that referenced this pull request Mar 28, 2017

Use more S3 in task ops (#1711)

448c664

* re-implemented #1474 * add default for getTaskDesc * missing NAMESPACE * fix for plotViperCharts * fix costsens bug * getTaskCosts only for Tasks

mllg deleted the add_asserts_to_task_operators branch April 18, 2017 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add more asserts to task operator #1474

add more asserts to task operator #1474

berndbischl commented Feb 2, 2017 •

edited

Loading

berndbischl commented Feb 2, 2017 •

edited

Loading

berndbischl commented Feb 2, 2017

larskotthoff commented Feb 2, 2017

berndbischl commented Feb 2, 2017

berndbischl commented Feb 2, 2017

berndbischl commented Feb 2, 2017

larskotthoff Feb 2, 2017

berndbischl Feb 3, 2017

larskotthoff Feb 2, 2017

berndbischl commented Feb 3, 2017

larskotthoff commented Feb 3, 2017

mllg Feb 14, 2017

mllg Feb 14, 2017

mllg Feb 14, 2017

mllg Feb 14, 2017

mllg Feb 14, 2017

mllg commented Mar 19, 2017

berndbischl commented Mar 19, 2017

berndbischl commented Mar 19, 2017

mllg commented Mar 19, 2017

		@@ -252,6 +253,8 @@ getTaskTargets.CostSensTask = function(task, recode.target = "no") {
		#' head(getTaskData(task, subset = 1:100, recode.target = "01"))
		getTaskData = function(task, subset, features, target.extra = FALSE, recode.target = "no") {

add more asserts to task operator #1474

add more asserts to task operator #1474

Conversation

berndbischl commented Feb 2, 2017 • edited Loading

berndbischl commented Feb 2, 2017 • edited Loading

berndbischl commented Feb 2, 2017

larskotthoff commented Feb 2, 2017

berndbischl commented Feb 2, 2017

berndbischl commented Feb 2, 2017

berndbischl commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berndbischl commented Feb 3, 2017

larskotthoff commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mllg commented Mar 19, 2017

berndbischl commented Mar 19, 2017

berndbischl commented Mar 19, 2017

mllg commented Mar 19, 2017

berndbischl commented Feb 2, 2017 •

edited

Loading

berndbischl commented Feb 2, 2017 •

edited

Loading