search

mlr-archive / mlr-tutorial

The mlr package online tutorial
http://mlr-org.github.io/mlr/
20 stars 11 forks source link

reflect changes to generatePartialDependenceData and removal of fanova #142

Closed zmjones closed 6 years ago

zmjones commented 6 years ago

should fix issue #141

zmjones commented 6 years ago

this looks like an unrelated failure?

larskotthoff commented 6 years ago

This removes all the examples for plotting partial dependence data. Is that intentional?

zmjones commented 6 years ago

nope unintentional because i was being hasty

zmjones commented 6 years ago

thanks for checking it

pat-s commented 6 years ago

@zmjones Since this is blocking the build of mlr-tutorial, it would be nice if you could provide a fix within the next days :1st_place_medal:

zmjones commented 6 years ago

@pat-s the failure is unrelated to any of my changes it looks like. master is failing too right? looks like the bug is on the benchmark experiments page.

pat-s commented 6 years ago

Currently there are multiple problems. The one related to benchmark_experiments is already fixed in #139. If you inspect the travis build there, you will also find the problem related to partial dependence.

You can merge this PR into yours and when your problem is fixed, the build should pass.

zmjones commented 6 years ago

@pat-s yep you are right, my bad. i only looked at the first error i saw in the travis log. thanks for pointing that out. should pass now.

larskotthoff commented 6 years ago

There's still some stuff on partial dependence that's being removed, not only the functional ANOVA stuff. Is that intentional?

pat-s commented 6 years ago

@zmjones Your build can only pass if you merge the changes from #139.

I've merged yours now into #139 to see if the build passes.

Anyhow, as @larskotthoff points out, I do not know if the simple removal of the problematic part is the way to go here? Idk what the reason for the partial dependence change was..

pat-s commented 6 years ago

Seems visualization.Rmd is also affected: https://travis-ci.org/mlr-org/mlr-tutorial/builds/336651787?utm_source=github_status&utm_medium=notification

zmjones commented 6 years ago

i rewrote the underlying estimation functions and abstracted them into a separate package. they are faster and more efficient now. i removed some functionality which made things more complicated, was not really necessary, and hard for users to understand, and i removed fanova which is too slow as it was to be used on anything but toy examples.

i just ran through visualization.Rmd locally and didn't get any errors...

larskotthoff commented 6 years ago

Ok, merging this.

pat-s commented 6 years ago

@larskotthoff the benchmark.Rmd related issues were not merged into this branch, hence we are still stuck at the same point. See #139.

To solve the visualiuation.Rmd problem that is only visible if #139 is solved, @zmjones opened https://github.com/mlr-org/mlr/pull/2153.

Still, after this is merged, this PR here (or master then) needs to merge the changes from #139 to pass..

So I would recommend to merge https://github.com/mlr-org/mlr/pull/2153 (if its ok) and then #139 should pass as it already has the changes from this PR here.