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

R-package cv #561

Merged
merged 14 commits into from
Jan 29, 2019
Merged

R-package cv #561

merged 14 commits into from
Jan 29, 2019

Conversation

brsoyanvn
Copy link
Contributor

Implemented catboost.cv function in R-package.

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en.

Copy link
Collaborator

@nikitxskv nikitxskv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several comments.

catboost/R-package/R/catboost.R Outdated Show resolved Hide resolved
catboost/R-package/src/catboostr.cpp Outdated Show resolved Hide resolved
@brsoyanvn
Copy link
Contributor Author

@nikitxskv I fixed the issues you pointed me to.

@nikitxskv
Copy link
Collaborator

@arcadia-devtools Ship it!

@annaveronika
Copy link
Contributor

We are currently planning our next release. For now the planned date is Friday. It would be super nice to have this pr in. Please do the changes asap.

@arcadia-devtools
Copy link
Collaborator

@nikitxskv, internal review request created: 645875

@annaveronika
Copy link
Contributor

There is a build error

$(SOURCE_ROOT)/catboost/R-package/src/catboostr.cpp:439:5: error: no matching function for call to 'CrossValidate'
CrossValidate(
^~~~~~~~~~~~~
$(SOURCE_ROOT)/catboost/libs/train_lib/cross_validation.h:131:6: note: candidate function not viable: no known conversion from 'NCB::TDataProvider' (aka 'TDataProviderTemplateNCB::TObjectsDataProvider') to 'NCB::TDataProviderPtr' (aka 'TIntrusivePtr<TDataProviderTemplateNCB::TObjectsDataProvider >') for 4th argument
void CrossValidate(
^
1 error generated.

@annaveronika
Copy link
Contributor

@brsoyanvn There is a build error, could you fix it? Do you see it when building?

@Evgueni-Petrov-aka-espetrov
Copy link
Contributor

Hopefully, the error will go away if dereferencing is removed...

@brsoyanvn
Copy link
Contributor Author

@annaveronika There is no any build error now

@annaveronika
Copy link
Contributor

@brsoyanvn There are conflicts, could you resolve them?

@annaveronika
Copy link
Contributor

@brsoyanvn Do you plan to finish this?

@brsoyanvn
Copy link
Contributor Author

@annaveronika sorry for a delay, I was busy with other issues. I resolved the conflicts

@annaveronika
Copy link
Contributor

@arcadia-devtools Ship it!

@arcadia-devtools
Copy link
Collaborator

@nikitxskv, internal review request created: 675767

@arcadia-devtools arcadia-devtools merged commit 3391b12 into catboost:master Jan 29, 2019
arcadia-devtools pushed a commit that referenced this pull request Jan 29, 2019
ref:aa3f69a58bee11d5e581ceed1fe056a8ea626655
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.

5 participants