Skip to content

Add cutting eval_period size if it is larger than step size#653

Merged
arcadia-devtools merged 12 commits into
catboost:masterfrom
AntPon:cut_eval_period_size
Feb 26, 2019
Merged

Add cutting eval_period size if it is larger than step size#653
arcadia-devtools merged 12 commits into
catboost:masterfrom
AntPon:cut_eval_period_size

Conversation

@AntPon

@AntPon AntPon commented Feb 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@annaveronika

annaveronika commented Feb 2, 2019

Copy link
Copy Markdown
Contributor

please agree to the CLA, see examples in other pr-s

@annaveronika

annaveronika commented Feb 2, 2019

Copy link
Copy Markdown
Contributor

please resolve conflicts

Noxoomo
Noxoomo reviewed Feb 2, 2019
View reviewed changes
Comment thread catboost/python-package/ut/medium/test.py Outdated
test_pool = Pool(data=test.iloc[:,1:], label=test.iloc[:,0])
model = CatBoostClassifier(iterations=100)
model.fit(train_pool, eval_set=test_pool)
model.eval_metrics(test_pool, ['AUC', 'Recall'], eval_period=200)

@Noxoomo Noxoomo Feb 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

было бы неплохое еще тест, в котором указаны begin и end

Noxoomo
Noxoomo reviewed Feb 2, 2019
View reviewed changes
Comment thread catboost/python-package/ut/medium/test.py
@@ -18,12 +18,15 @@
Pool,

@Noxoomo Noxoomo Feb 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

надо обновить репо катбуста до последней версии
сейчас нельзя замержить из-за того, что файл в репо более свежий и автоматически конфликты не разрешить

@annaveronika

annaveronika commented Feb 6, 2019

Copy link
Copy Markdown
Contributor

@arcadia-devtools Ship it!

@arcadia-devtools

arcadia-devtools commented Feb 6, 2019

Copy link
Copy Markdown
Collaborator

@annaveronika, internal review request created: 709157

@annaveronika

annaveronika commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

There are some pep8 check errors:

arcadia/catboost/python-package/ut/medium/test.py:3670:1: W293 blank line contains whitespace

^
arcadia/catboost/python-package/ut/medium/test.py:3671:1: E302 expected 2 blank lines, found 1
def test_eval_period_size():
^
arcadia/catboost/python-package/ut/medium/test.py:3673:40: E231 missing whitespace after ','
    train_pool = Pool(data=train.iloc[:,1:], label=train.iloc[:,0])
                                       ^
arcadia/catboost/python-package/ut/medium/test.py:3673:64: E231 missing whitespace after ','
    train_pool = Pool(data=train.iloc[:,1:], label=train.iloc[:,0])
                                                               ^
arcadia/catboost/python-package/ut/medium/test.py:3674:38: E231 missing whitespace after ','
    test_pool = Pool(data=test.iloc[:,1:], label=test.iloc[:,0])
                                     ^
arcadia/catboost/python-package/ut/medium/test.py:3674:61: E231 missing whitespace after ','
    test_pool = Pool(data=test.iloc[:,1:], label=test.iloc[:,0])
                                                            ^
arcadia/catboost/python-package/ut/medium/test.py:3677:70: W291 trailing whitespace
    model.eval_metrics(test_pool, ['AUC', 'Recall'], eval_period=200)
                                                                     ^
arcadia/catboost/python-package/ut/medium/test.py:3679:1: E302 expected 2 blank lines, found 1
def test_model_comparison():
^

@annaveronika

annaveronika commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

Please use for tests local files only.
amazon() function downloads the dataset, so it needs access to internet. It is better if tests don't need access to internet. You can use any dataset from other tests in the test.py file.

@annaveronika

annaveronika commented Feb 14, 2019

Copy link
Copy Markdown
Contributor

@AntPon Do you plan to proceed with the pr?

@andrey-khropov

andrey-khropov commented Feb 26, 2019

Copy link
Copy Markdown
Member

@arcadia-devtools Ship it!

@catboost catboost deleted a comment from arcadia-devtools Feb 26, 2019
@arcadia-devtools

arcadia-devtools commented Feb 26, 2019

Copy link
Copy Markdown
Collaborator

@andrey-khropov, internal review request created: 737519

@catboost catboost deleted a comment from arcadia-devtools Feb 26, 2019
@arcadia-devtools
arcadia-devtools merged commit dbd2372 into catboost:master Feb 26, 2019
@arcadia-devtools

arcadia-devtools commented Feb 26, 2019

Copy link
Copy Markdown
Collaborator

Check 737519 failed: 1 test(s) broken

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@Noxoomo Noxoomo Noxoomo left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AntPon @annaveronika @arcadia-devtools @andrey-khropov @Noxoomo