Skip to content

R-package cv#561

Merged
arcadia-devtools merged 14 commits into
catboost:masterfrom
brsoyanvn:r_package_cv
Jan 29, 2019
Merged

R-package cv#561
arcadia-devtools merged 14 commits into
catboost:masterfrom
brsoyanvn:r_package_cv

Conversation

@brsoyanvn

Copy link
Copy Markdown
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.

@nikitxskv nikitxskv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Several comments.

Comment thread catboost/R-package/R/catboost.R Outdated
Comment thread catboost/R-package/src/catboostr.cpp Outdated
@brsoyanvn

Copy link
Copy Markdown
Contributor Author

@nikitxskv I fixed the issues you pointed me to.

@nikitxskv

Copy link
Copy Markdown
Collaborator

@arcadia-devtools Ship it!

@annaveronika

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

@nikitxskv, internal review request created: 645875

@annaveronika

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@Evgueni-Petrov-aka-espetrov

Copy link
Copy Markdown
Contributor

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

@brsoyanvn

Copy link
Copy Markdown
Contributor Author

@annaveronika There is no any build error now

@annaveronika

Copy link
Copy Markdown
Contributor

@brsoyanvn There are conflicts, could you resolve them?

@annaveronika

Copy link
Copy Markdown
Contributor

@brsoyanvn Do you plan to finish this?

@brsoyanvn

Copy link
Copy Markdown
Contributor Author

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

@annaveronika

Copy link
Copy Markdown
Contributor

@arcadia-devtools Ship it!

@arcadia-devtools

Copy link
Copy Markdown
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