Expression handling #1126

kerschke · 2016-08-11T19:19:09Z

This PR allows to finally use task dependent expressions in the learner (see #767).

larskotthoff · 2016-08-12T08:31:10Z

Thanks Pascal.

Please document prominently what parameters are available. This should appear not only in the documentation for evaluateLearner, but also makeLearner, setHyperPars and anywhere else where people can set hyperparameters.
Why is the task itself in the dictionary?
In the unit tests, instead of checking the values for randomForest (which are specified somewhere else), could you please use a hand-constructed expression? Otherwise this test will fail for no good reason if the values in randomForest are changed.

kerschke · 2016-08-12T08:38:43Z

Please document prominently what parameters are available. This should appear not only in the documentation for evaluateLearner, but also makeLearner, setHyperPars and anywhere else where people can set hyperparameters.

Ok, I'll do that :)

Why is the task itself in the dictionary?

That's a decision we've made with @mllg and @berndbischl. The main reason is that now the user can use any expression which takes any information from the task. I mean, you probably wouldn't want to do expression(task) but expression(task$...).

In the unit tests, instead of checking the values for randomForest (which are specified somewhere else), could you please use a hand-constructed expression? Otherwise this test will fail for no good reason if the values in randomForest are changed.

The tests already contain hand-constructed expressions, e.g. the rpart with hand-constructed hyperparameters.. The reason for having the randomForest in there, is actually to check whether it also works with the parameter setup that is defined from our side within the definition of the learner. And if those values are changed by someone of us, well then that person should also update the unit tests accordingly. Right?

larskotthoff · 2016-08-12T08:54:53Z

> Why is the task itself in the dictionary? That's a decision we've made with @mllg and @berndbischl. The main reason is that now the user can use any expression which takes any information from the task. I mean, you probably wouldn't want to do `expression(task)` but `expression(task$...)`.

Ok, but then this should be documented as well.

> In the unit tests, instead of checking the values for randomForest (which > are specified somewhere else), could you please use a hand-constructed > expression? Otherwise this test will fail for no good reason if the values > in randomForest are changed. The tests already contain hand-constructed expressions, e.g. the rpart with hand-constructed hyperparameters.. The reason for having the randomForest in there, is actually to check whether it also works with the parameter setup that is defined from our side within the definition of the learner. And if those values are changed by someone of us, well then that person should also update the unit tests accordingly. Right?

I don't see the benefit of testing something external -- what does this test that the hand-constructed example doesn't test? It introduces unnecessary coupling and I'm sure that we'll always forget this and then have to come back to a PR later and fix things.

kerschke · 2016-08-14T07:43:10Z

the documentation has been updated.. please review again and then merge ;)

mllg · 2016-08-15T14:15:31Z

Reviewed. Everything okay. @berndbischl -> ok to merge?

kerschke · 2016-08-15T21:01:37Z

@berndbischl: ping..

berndbischl · 2016-08-15T21:06:33Z

you do know that i am on holiday right?

kerschke · 2016-08-15T21:07:06Z

yes, and I see your working on all other ends of the package during your holiday ;)

berndbischl · 2016-08-15T21:12:11Z

evaluateParset should be called evaluateParamSet. We have to be consistent with the names

kerschke · 2016-08-15T21:13:43Z

Then, we're mixing up the names from ParamHelpers (evaluateParamSet) and mlr (evaluateParSet). But if that's the only thing that you don't like, I can fix that tomorrow..

berndbischl · 2016-08-26T10:00:11Z

@berndbischl @larskotthoff Last call

i am on holiday pls wait at least until monday.

schiffner · 2016-08-26T12:21:59Z

Where can I find your tutorial stuff? Can you link it here?

In case that you haven't found it already: mlr-archive/mlr-tutorial#49

larskotthoff · 2016-08-26T20:09:32Z

Looks good to me.

mllg · 2016-09-05T20:03:20Z

@berndbischl ping ping ping

kerschke · 2016-09-19T07:11:26Z

@berndbischl what's your status on this one? i mean ping, ping, ping ;)

# Conflicts: # DESCRIPTION # R/Learner_properties.R # tests/testthat/test_base_measures.R

jakob-r · 2017-02-02T15:11:21Z

Test fail because of roxygen2 6.0.0

jakob-r · 2017-02-16T09:12:11Z

@berndbischl please review

mllg · 2017-02-22T09:26:04Z

@berndbischl ping

jakob-r · 2017-02-23T16:06:11Z

@berndbischl ping

mb706 · 2017-02-27T04:59:55Z

R/train.R

@@ -31,6 +31,10 @@
 train = function(learner, task, subset, weights = NULL) {
  learner = checkLearner(learner)
  assertClass(task, classes = "Task")
+  if (hasExpression(learner)) {
+    dict = getTaskDictionary(task = task)


The dict should somehow contain information about subset. The learner only sees the subset task, and probably expects the parameters to behave accordingly.

I would assume that the task is already subsetted?

Subsetting happens in trainLearner (e.g.)

Of course. I remember... what I don't remember is the motivation behind this though.

jakob-r · 2017-03-02T12:11:09Z

All reviews were postive. @mb706 comment can be implemented later in another PR.

berndbischl · 2017-03-03T08:48:22Z

@jakob-r @mllg
can we please comment on:
a) why you merged something that apparently seems wrong? or please tell me why @mb706 is not correct.
if the the subsetting is done later, the the "n" symbol will we incorrect in the dict?

if thats the case, that creates invalid results.

b) this is a base change. who where the 2 core members who reviewed positively?

berndbischl · 2017-03-03T08:59:48Z

@jakob-r
i am sorry, i wanted to avoid this, but if it takes 3 min (!) to check EXACTLY what @mb706 said in a test.

load_all()
task = makeClassifTask(data = iris, target = "Species")
dict = getTaskDictionary(task = task)
lrn1 = makeLearner("classif.rpart", minsplit = expression(n))
print(getHyperPars(lrn1))
m = train(lrn1, task)
print(m$learner.model$control$minsplit)
m = train(lrn1, task, subset = 1:100)
print(m$learner.model$control$minsplit # is 150! wrong

what do you think happens now to people who use that code, and generate results from it?

will revert now

jakob-r · 2017-03-03T09:32:41Z

n is documented to be the size of the Task and not of any subset.

R/evaluateParamExpressions.R

\item{\code{n}:} the number of observations in the task

In other words this example just assumes something that is never mentioned in the documentation.

berndbischl · 2017-03-03T09:42:50Z

In other words this example just assumes something that is never mentioned in the documentation.

we can discuss this on hangout, but you cannot be serious

berndbischl · 2017-03-03T09:44:51Z

maybe as reviewer it is also your obligation to think about whether the docs make sense? whether the semantics are good and proper?

do you want to argue they are, here? that the example i posted is OK? you would like to have that behavior in your own experiments? that the defaults in the other underlying packages work this way?

kerschke added prio-high pr-ready for merge (?) pr-please review and removed pr-ready for merge (?) labels Aug 11, 2016

kerschke mentioned this pull request Aug 14, 2016

tutorial pages for expression handling mlr-archive/mlr-tutorial#49

Open

kerschke and others added 11 commits August 15, 2016 15:36

initial version of expression-learners

17fe5e0

allow expressions within tuning param sets

b56128d

updated documentation of expression-related files

62d6bdb

fixing naming issues

8b20d6f

further doc fixes

f85ad87

remove dict argument

a13c4eb

updating man-pages

eb9ec14

rm dict_template

eaea99e

better documentation of expression-related functions

7227a9c

removed ParamHelpers:: as it is not necessary

00d92f8

cleanup; added test

e0b4728

mllg force-pushed the expression branch from 4f503e8 to e0b4728 Compare August 15, 2016 13:38

removed duplicated PH dep

43e2f4f

kerschke added pr-ready for merge (?) and removed pr-please review labels Aug 15, 2016

mllg approved these changes Sep 19, 2016

View reviewed changes

Merge branch 'master' into expression

d72e6fd

# Conflicts: # DESCRIPTION # R/Learner_properties.R # tests/testthat/test_base_measures.R

jakob-r and others added 5 commits February 2, 2017 16:41

added assertion for Task

10d81d5

Merge branch 'master' into expression

42c92e4

Merge remote-tracking branch 'origin/master' into expression

7eb94f5

Merge branch 'expression' of github.com:mlr-org/mlr into expression

5144137

docs, mini cleanup

cb5fa80

larskotthoff mentioned this pull request Feb 26, 2017

randomForest hangs when nodesize >= sampsize #1557

Closed

mb706 reviewed Feb 27, 2017

View reviewed changes

jakob-r closed this Mar 2, 2017

jakob-r reopened this Mar 2, 2017

jakob-r merged commit 416305d into master Mar 2, 2017

berndbischl mentioned this pull request Mar 3, 2017

fix and finish expression handling #1569

Closed

jakob-r mentioned this pull request Mar 6, 2017

Expression handling in mlr #1576

Closed

pat-s mentioned this pull request Jun 17, 2019

expressions in learner params #767

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expression handling #1126

Expression handling #1126

kerschke commented Aug 11, 2016

larskotthoff commented Aug 12, 2016

kerschke commented Aug 12, 2016 •

edited

Loading

larskotthoff commented Aug 12, 2016 via email

kerschke commented Aug 14, 2016

mllg commented Aug 15, 2016

kerschke commented Aug 15, 2016

berndbischl commented Aug 15, 2016

kerschke commented Aug 15, 2016

berndbischl commented Aug 15, 2016

kerschke commented Aug 15, 2016

berndbischl commented Aug 26, 2016

schiffner commented Aug 26, 2016

larskotthoff commented Aug 26, 2016

mllg commented Sep 5, 2016

kerschke commented Sep 19, 2016

jakob-r commented Feb 2, 2017

jakob-r commented Feb 16, 2017

mllg commented Feb 22, 2017

jakob-r commented Feb 23, 2017

mb706 Feb 27, 2017

jakob-r Feb 27, 2017

mb706 Feb 27, 2017

jakob-r Feb 28, 2017

jakob-r commented Mar 2, 2017

berndbischl commented Mar 3, 2017 •

edited

Loading

berndbischl commented Mar 3, 2017

jakob-r commented Mar 3, 2017 •

edited

Loading

berndbischl commented Mar 3, 2017

berndbischl commented Mar 3, 2017

Expression handling #1126

Expression handling #1126

Conversation

kerschke commented Aug 11, 2016

larskotthoff commented Aug 12, 2016

kerschke commented Aug 12, 2016 • edited Loading

larskotthoff commented Aug 12, 2016 via email

kerschke commented Aug 14, 2016

mllg commented Aug 15, 2016

kerschke commented Aug 15, 2016

berndbischl commented Aug 15, 2016

kerschke commented Aug 15, 2016

berndbischl commented Aug 15, 2016

kerschke commented Aug 15, 2016

berndbischl commented Aug 26, 2016

schiffner commented Aug 26, 2016

larskotthoff commented Aug 26, 2016

mllg commented Sep 5, 2016

kerschke commented Sep 19, 2016

jakob-r commented Feb 2, 2017

jakob-r commented Feb 16, 2017

mllg commented Feb 22, 2017

jakob-r commented Feb 23, 2017

mb706 Feb 27, 2017

Choose a reason for hiding this comment

jakob-r Feb 27, 2017

Choose a reason for hiding this comment

mb706 Feb 27, 2017

Choose a reason for hiding this comment

jakob-r Feb 28, 2017

Choose a reason for hiding this comment

jakob-r commented Mar 2, 2017

berndbischl commented Mar 3, 2017 • edited Loading

berndbischl commented Mar 3, 2017

jakob-r commented Mar 3, 2017 • edited Loading

berndbischl commented Mar 3, 2017

berndbischl commented Mar 3, 2017

kerschke commented Aug 12, 2016 •

edited

Loading

berndbischl commented Mar 3, 2017 •

edited

Loading

jakob-r commented Mar 3, 2017 •

edited

Loading