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

enabled quantile regression in gbm #2603

Merged
merged 14 commits into from Jun 25, 2019

Conversation

@bthieurmel
Copy link
Contributor

commented Jun 14, 2019

We are always happy to receive pull requests.

Please make sure you have read our coding guidelines:
https://github.com/mlr-org/mlr/wiki/mlr-Coding-Guidelines

This especially means that you have understood:

  • The style guide - our lintr will provide you feedback on this
  • How to run tests locally. Yes, travis will also run them for you, bad it is annoying to wait for this.
  • How to run R CMD CHECK locally. See point before.

Also it's helpful to get into direct contact with the suggested reviewers to get help, getting your PR merged.
You might want to join our slack at:
https://mlr-org.slack.com

R/RLearner_regr_gbm.R Outdated Show resolved Hide resolved
R/RLearner_regr_gbm.R Outdated Show resolved Hide resolved
@pat-s
Copy link
Member

left a comment

Thanks. Please see my comments. Also please add an entry to NEWS.md.

R/RLearner_regr_gbm.R Outdated Show resolved Hide resolved
R/RLearner_regr_gbm.R Outdated Show resolved Hide resolved
R/RLearner_regr_gbm.R Show resolved Hide resolved
NEWS.md Outdated
@@ -11,6 +11,7 @@
See `?regr.randomForest` for more details.
`regr.ranger` relies on the functions provided by the package ("jackknife" and "infjackknife" (default))
(@jakob-r, #1784)
- `regr.gbm` now supports `quantile distribution`

This comment has been minimized.

Copy link
@pat-s

pat-s Jun 17, 2019

Member

Also add your username and the PR number please.

@pat-s

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Thanks. Only one comment left from my side.

Please take a look why the test is failing and tell us if you need help.

Looks almost ready to merge 👍

@bthieurmel

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Seems to be ok (missing ! and so don't pass weights information....)

@pat-s

pat-s approved these changes Jun 17, 2019

gbm::gbm(f, data = getTaskData(.task, .subset), ...)

params = list(...)
if("alpha" %in% names(params)) {

This comment has been minimized.

Copy link
@larskotthoff

larskotthoff Jun 17, 2019

Contributor

Space after if and I would check whether params$alpha is NULL for consistency with how it's set below.

gbm::gbm(f, data = getTaskData(.task, .subset), weights = .weights, ...)
alpha = 0.5
}
if(params$distribution %in% "quantile") {

This comment has been minimized.

Copy link
@larskotthoff

larskotthoff Jun 17, 2019

Contributor

Space after if and should that check be simply ==?

@pat-s

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

For style, you can simply apply the mlr style guide. (in case you haven't already)

@pat-s pat-s added the reply-needed label Jun 19, 2019

@pat-s

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

@larskotthoff If you approve, feel free to merge. We can run the style fixer afterwards. Applying it on a forked branch is not easy.

@larskotthoff

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Merging.

@larskotthoff larskotthoff merged commit 98b0b71 into mlr-org:master Jun 25, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.