aorsf learner

[bcjaeger]

New Learner

This pull request adds LearnerSurvAorsf with the unique key surv.aorsf from package {aorsf}.

I have...

• Created a learner with the create_learner template or created one manually and it follows the identical layout.
• Added .train and .predict private methods
• Added ParamSet and if applicable param_vals
• Added importance public method and completed the documentation for this - !!!Delete if not applicable!!!
• Added oob_error public method and completed the documentation for this - !!!Delete if not applicable!!!
• Added references and not left the tag blank - !!!Delete if not applicable!!!
• Removed all 'FIXME's

Dependencies

I have...

• Added more than one dependency to suggests - If ticked, explain why below.
• Added no dependencies to imports.
• Added dependencies to remotes - If ticked, explain why below.

I have added {aorsf} to suggests

Tests

I have...

• Created a testthat file and it passes locally.
• Created a paramtest file with tests for all calls in both my .train and .predict methods and it passes locally.

Cleaning

I have run...

• devtools::document(roclets = c('rd', 'collate', 'namespace'))
• lintr::lint_package()

And

• Updated NEWS.md

Maintenance and Contributions

This PR will only be merged if the following is filled in and ticked:

• bcjaeger agrees to maintain this learner going forward and agrees that this learner will be removed from mlr3extralearners if: i) the build remains broken for more than one month with no effort to fix it; or ii) is broken for more than three months.

The maintainer of the learner can be changed at any time by editing the '@author' tag and creating a pull request.

Finally make sure you get credit and add yourself to the DESCRIPTION file (optionally add a note to say which learner(s) you implemented):

• I have added myself (and anyone else who contributed) to the end of the "Authors" field in the DESCRIPTION file with role "ctb"

Comments

Hello! I am following up on #198. aorsf has undergone review at rOpenSci¹ and is stable. It was also recently uploaded to CRAN.

¹Technically, the rOpenSci issue is still open (ropensci/software-review#532), but I anticipate the editor will be closing it sometime this week.

[sebffischer]

Thank you for contributing!

To keep you updated:
I wanted to merge some small fixes first. I will update your branch and review this PR hopefully later today

[bcjaeger]

Awesome! Thanks for letting me know - I'm happy to make updates based on your review.

[sebffischer]

Looking good! Most of my comments are minor details (where we should have documented better) or styling. Don't hesitate to ask if something is unclear.

Do you think we should add a warning when using your slower implementation to make users aware that a faster version exists?

[bcjaeger]

Thank you for reviewing so carefully - the suggested edits are great!

I will check out and resolve each of the conversations above, and let you know when I've gotten everything wrapped up.

[sebffischer]

Great.
For some reason it cannot find the aorsf but I am not sure why. I will check tomorrow.

[bcjaeger]

Do you think we should add a warning when using your slower implementation to make users aware that a faster version exists?

This is a great idea - should I put a warning() about this in the initialize function for obliqueRSF learners?

[bcjaeger]

aorsf should be on CRAN (https://cran.r-project.org/web/packages/aorsf/index.html) so I haven't added the remotes part yet. But if the CRAN link still doesn't work I am happy to add a remote.

[sebffischer]

Do you think we should add a warning when using your slower implementation to make users aware that a faster version exists?

This is a great idea - should I put a warning() about this in the initialize function for obliqueRSF learners?

Yes I would do it like that.

[sebffischer]

Thanks for fixing most of the issues!
I have left some more comments.

[sebffischer]

I think the problem with the installation is in the CI and not with your package

[sebffischer]

So your tests are passing!
The errors are still problems with the CI that I have to address, but when you address the issues I raised your PR looks good to be merged :)

[bcjaeger]

I think I've addressed all of the comments now.

One thing I am trying to figure out: I think predict.orsf_fit() is exported from aorsf, but when I use aorsf::predict.orsf_fit() in run_paramtest() I get an error saying predict.orsf_fit() is not exported. Codefactor does not like me using :::, so I've modified the paramtest file for aorsf, using predict() in fun_list instead of aorsf:::predict.orsf_fit(). The test passes for me locally with this workaround, and I don't think it upsets codefactor. What do you think?

[sebffischer]

Sorry for the late reply. Your workaroud is fine. In the tests I think codefactor issues can be ignore, exactly for the reason you mention.

[sebffischer]

I formatted your code using the mlr3 style, which addressed the only comments that I would have made.

Thanks for contributing and for addressing all my comments ! :)

[sebffischer]

lgtm!

[bcjaeger]

Awesome - thank you! =]