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

WIP: Add SampleId as a main name for sample id column in a column description file #655

Merged
merged 5 commits into from
Feb 26, 2019
Merged

Conversation

daskol
Copy link
Contributor

@daskol daskol commented Feb 2, 2019

No description provided.

@annaveronika
Copy link
Contributor

please agree to cla, see examples in other pr-s

@annaveronika
Copy link
Contributor

@arcadia-devtools Ship it!

@annaveronika
Copy link
Contributor

Please add a test that shows that both DocId and SampleId work

@@ -70,6 +70,9 @@ namespace {
if (type == "Target") {
type = "Label";
}
if (type == "DocId") {
Copy link
Member

@andrey-khropov andrey-khropov Feb 2, 2019

Choose a reason for hiding this comment

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

The same logic should be added where output columns names are parsed in eval_result.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, it is tricky. I was expected that column_descriptor/ is the only place where string names of column types are used. I think it would be better to pass TVector<TColumn> instead of TVector<TString> to OutputEvalResultToFile().

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to pass TVector instead of TVector to OutputEvalResultToFile().

I agree but this is more involved refactoring outside of this particular change.

@arcadia-devtools
Copy link
Collaborator

@annaveronika, internal review request created: 701585

@annaveronika
Copy link
Contributor

@arcadia-devtools Ship it!

@arcadia-devtools
Copy link
Collaborator

@annaveronika, internal review request created: 709727

@annaveronika
Copy link
Contributor

@daskol You need to agree to our cla (see examples in other pr-s), otherwise we will not be able to merge your pr.

@annaveronika
Copy link
Contributor

@daskol Do you plan to proceed with the pr?

@andrey-khropov
Copy link
Member

@arcadia-devtools Ship it!

@arcadia-devtools
Copy link
Collaborator

@andrey-khropov, internal review request created: 737216

@arcadia-devtools arcadia-devtools merged commit 37bbf31 into catboost:master Feb 26, 2019
@daskol daskol deleted the feature/sample-id branch February 26, 2019 19:08
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.

4 participants