pfistfl commented 7 years ago

This is a new PR for the functional data section, as I could not properly rebase the original branch.

101 contains some of the discussion and the original pr

pfistfl commented 7 years ago

@SteveBronder This does still not pass. Do you have any idea?

pfistfl commented 6 years ago

Maybe @jakob-r can help? I have no idea why I keep getting the missing kknn learner error.

The error is

Knitting file 'nested_resampling.Rmd' ...
Quitting from lines 312-338 () 
Error in requirePackages(package, why = stri_paste("learner", id, sep = " "),  : 
  For learner classif.kknn please install the following packages: kknn
Calls: lapply ... addClasses -> makeRLearnerInternal -> requirePackages -> stopf
Execution halted

pat-s commented 6 years ago

@pfistfl I face similar problems locally a lot when running ./build. Its most often solved by explictly adding a library() call into the problematic code chunk.

For travis, this does of course not apply and currently I think its random travis behaviour.

jakob-r commented 6 years ago

So the error just occurs on cran randomly but locally everything works fine?

pat-s commented 6 years ago

@jakob-r The error occured locally for me while everything was fine on travis.

Its a strange one because the package that is said to be not installed is definitely installed. If I added an explicit libray() call into the offending code chunk, everything was working fine. Maybe this will also help in this case..

SteveBronder commented 6 years ago

Maybe this is a maximum number of DLLs reached error in disguise? Can you try setting this environment variable

R_MAX_NUM_DLLS=300

and re-running. If it compiles on your local then it's not this issue (probably)

pfistfl commented 6 years ago

@SteveBronder

Maybe this is a maximum number of DLLs reached

I think this must be the case. Reasons:

The error is returned for tutorial sections I have not touched at all
The offending package was kknn before, and now it is earth (changed with new merged pr's)
I got the same error locally today, and setting R_MAX_NUM_DLLS=300 solved the issue.

pfistfl commented 6 years ago

The functional data section seems to load ~14 dlls. This puts the number of .dlls to over 100. I added a call to length(getLoadedDLLs()) to build. See: out.txt

SteveBronder commented 6 years ago

Yeah in that case you should be able to set the above environment variable on travis and everything will compile fine

pfistfl commented 6 years ago

Thanks a lot, I did not know it would be that easy. Hope it passes now.

SteveBronder commented 6 years ago

Yep! I brought this issue up to R devel a while ago and one guy was nice enough to have make a very clean / simple patch.

If you want another set of eyes to review it before you merge I should have time this week

pfistfl commented 6 years ago

Yeah that'd be awesome. I think some people already reviewed the old PR, but reviewing can not hurt. Other than that I think we are finally ready to merge.

pfistfl commented 6 years ago

Thanks a lot, I really found your suggestions helpful. I had a hard time describing the wavelets and fourier transform in a few lines (heck, I could not even find a short, concise explanation on the internet). I added links to tutorials instead, hope that helps.

pfistfl commented 6 years ago

@jakob-r @schiffner

I had to change the Travis config. Pending your ok, I think we are ready for merge

pfistfl commented 6 years ago

Thanks a lot! I should have checked the links!

pfistfl commented 6 years ago

So basically the builds now break because of travis's time limit. We had build times of around 49 mins before, so this is to be expected I guess.

Is there anything I can do? Set up travis build stages?

SteveBronder commented 6 years ago

If you are planning to set up build stages I would recommend looking over @pat-s work in #115

SteveBronder commented 6 years ago

Besides using s3 for build stages I'm not sure how to get around the timer for us. That alt is to help/wait for building in pkgdown within mlr #2123 which will probably save a lot of headache in the long run

pat-s commented 6 years ago

If you are planning to set up build stages I would recommend looking over @pat-s work in #115

I have no plans continuing here in the near future; focusing on the pkgdown approach. There we have a reduced build time currently (42 min) and adding a new section should work there.

I would not put work in the integration into the mkdocs version here if we switch within the next weeks.

But maybe @jakob-r has a different opinion on this?

pat-s commented 6 years ago

hi @pfistfl,

would you mind integrating your section in the pkgdown PR? You should have rights to add commits.

See https://github.com/pat-s/mlr#mlr-tutorial for a guide of the new approach.

pfistfl commented 6 years ago

@pat-s Hi, perfect, this sounds really good. I was waiting for that!

pfistfl commented 6 years ago

I am on it, I will create a PR into pat-s:master later.

One thing, I saw is that rpkgdown::serve_site() does not exist for me.

pat-s commented 6 years ago

One thing, I saw is that rpkgdown::serve_site() does not exist for me.

Oh, is that a mistake in the doc? It should of course be pkgdown::build_site

pfistfl commented 6 years ago

Hey, @pat-s, I pushed into pat-s:mlr/master because I could not create branches. Will you take it from there? If you need anything from me, let me know!

pat-s commented 6 years ago

Thanks, looks good. I just did some minor changes:

Start a new line for every sentence
Exact one empty line before and after a code chunk
One package reference was still in the mkdocs style
The list around "regression, classif" was indented and therefore misaligned. Not sure if its completely fixed now, please check again after the site is deployed
add to pdf version

I would say you can then finally close this PR :smile:

EDIT: As long as everybody else is fine with the content. I did not check in detail ;)

pat-s commented 6 years ago

I appears to be that section https://pat-s.github.io/mlr/articles/tutorial/devel/functional_data.html#constructing-a-learner needs the mxnet package installed to properly display the code output. However, the mxnet package is not available on CRAN. Was this ever an issue in mlr-tutorial? If not, you might wanna try to integrate this into the travis build @pfistfl The build process is not completely trivial though: http://mxnet.incubator.apache.org/install/index.html

Furthermore, could you maybe try to remove the single bullet points in this section and see if that improves readability? They look a bit misplaced and are probably not really needed due to the code breaks.

pfistfl commented 6 years ago

Hey @pat-s

Thank you very much!

I will iterate again.

I think the mxnet learners have been added very recently, and they are not connected to my tutorial.

I would just not show the warning for now. Are you ok with that?

Other than that, this should do in order to install mxnet:

  cran <- getOption("repos")
  cran["dmlc"] <- "https://s3-us-west-2.amazonaws.com/apache-mxnet/R/CRAN/"
  options(repos = cran)
  install.packages("mxnet")

pfistfl commented 6 years ago

Looks good to me now!

Closing here I guess

mlr-archive / mlr-tutorial

Add functional data section #125

101 contains some of the discussion and the original pr