Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search for baselearner in this package #98

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kenahoo
Copy link

@kenahoo kenahoo commented Mar 13, 2019

Search for baselearner in this package, not in the caller's namespace.

When baselearner is given as a string, it's limited (by match.arg) to the pre-defined choices c("bbs", "bols", "btree", "bss", "bns"). It doesn't make sense to require the caller to import that symbol when we know it's defined in the mboost package anyway.

In my case, I'm invoking mboost through a third-party package that does cross-validation, so it's impossible for me to import btree into that namespace. The only solution is to do a shotgun library(mboost) to export the btree symbol globally, but I'd like to avoid that.

I also noticed that the bname variable is never used, so I got rid of it.

Search for `baselearner` in this package, not in the caller's namespace.

When `baselearner` is given as a string, it's limited (by `match.arg`) to the pre-defined choices `c("bbs", "bols", "btree", "bss", "bns")`.  It doesn't make sense to require the caller to import that symbol when we know it's defined in the `mboost` package anyway.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 78.621% when pulling 708915f on kenahoo:patch-1 into f10b45c on boost-R:master.

@hofnerb
Copy link
Member

hofnerb commented Mar 22, 2019

Thanks a lot for the patch and sorry for my rather slow response. Time is always (too) scarce. Unfortunately I cannot check the PR on windows as Appveyor is causing problems. Hope it is fixed soon.

@kenahoo
Copy link
Author

kenahoo commented Apr 12, 2019

Hi @hofnerb , it looks like the Appveyor failure doesn't really have anything to do with this patch, it looks like some configuration problems:

travis-tool.sh install_deps
...
+ Rscript -e 'options(repos=c(CRAN="https://cran.rstudio.com")); remotes::install_deps(dependencies = TRUE, type="both")'
...
Error: (converted from warning) package 'biomaRt' is not available (for R version 3.5.3 Patched)
Execution halted
Command exited with code 1

I think it's trying to install biomaRt from CRAN, but it's a bioconductor package.

@kenahoo
Copy link
Author

kenahoo commented Apr 15, 2019

@hofnerb about the previous comment - what happens if you move the - travis-tool.sh install_deps line in appveyor.yml to be the last step in build_script, instead of the first step?

@hofnerb
Copy link
Member

hofnerb commented Apr 15, 2019

Still not working, now due to another issue related to BioC... I was hoping it is working out by the time by itself. Very annoying.

@kenahoo
Copy link
Author

kenahoo commented Apr 22, 2019

So does this mean no fixes can happen on mboost for the foreseeable future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants