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

[python][tests] migrate test_engine.py to pytest #3800

Merged
merged 4 commits into from
Jan 22, 2021

Conversation

thomasjpfan
Copy link
Contributor

Towards #3732

@jameslamb jameslamb changed the title [python][tests] Migrates tset_engine.py to pytest [python][tests] migrate test_engine.py to pytest Jan 21, 2021
@jameslamb jameslamb self-requested a review January 21, 2021 00:01
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this! I compared with https://www.textcompare.org/index.html?id=6008e19c6d59840017ac7a56, and found a few mistakes.

Also thank you for removing these extra unused metrics.

image

verbose_eval=False,
evals_result=evals_result)
ret = multi_logloss(y_test, gbm.predict(X_test))
assert ret < 6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert ret < 6
assert ret < 0.16

from https://www.textcompare.org/index.html?id=6008e19c6d59840017ac7a56

image

verbose_eval=False,
evals_result=evals_result)
ret = multi_logloss(y_test, gbm.predict(X_test))
assert ret < 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert ret < 3
assert ret < 0.23

from https://www.textcompare.org/index.html?id=6008e19c6d59840017ac7a56

image

"pred_early_stop_freq": 5,
"pred_early_stop_margin": 1.5}
ret = multi_logloss(y_test, gbm.predict(X_test, **pred_parameter))
assert ret < 8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert ret < 8
assert ret < 0.8

from https://www.textcompare.org/index.html?id=6008e19c6d59840017ac7a56

image

"pred_early_stop_freq": 5,
"pred_early_stop_margin": 5.5}
ret = multi_logloss(y_test, gbm.predict(X_test, **pred_parameter))
assert ret < 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert ret < 2
assert ret < 0.2

from https://www.textcompare.org/index.html?id=6008e19c6d59840017ac7a56

image

evals_result=evals_result,
init_model=init_gbm)
ret = multi_logloss(y_test, gbm.predict(X_test))
assert ret < 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert ret < 1
assert ret < 0.1

from https://www.textcompare.org/index.html?id=6008e19c6d59840017ac7a56

image

preds = cvb.predict(X_test)
avg_pred = np.mean(preds, axis=0)
ret = log_loss(y_test, avg_pred)
assert ret < 5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert ret < 5
assert ret < 0.15

https://www.textcompare.org/index.html?id=6008e19c6d59840017ac7a56

image

# fold averaging
avg_pred = np.mean(preds, axis=0)
ret = log_loss(y_test, avg_pred)
assert ret < 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert ret < 3
assert ret < 0.13

from https://www.textcompare.org/index.html?id=6008e19c6d59840017ac7a56

image

assert ret_origin == pytest.approx(ret)


@unittest.skipIf(not lgb.compat.PANDAS_INSTALLED, 'pandas is not installed')
Copy link
Collaborator

Choose a reason for hiding this comment

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

could this and other unittest.skipIf in this PR use pytest.importorskip like you did in #3798 ? So that we don't need to import unittest

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

@thomasjpfan Wow, you did it! So huge file! Thanks a lot for the help with it.

I've found only 3 typos, the rest looks just great!

tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
tests/python_package_test/test_engine.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Many thanks!

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Oh sorry to hold this up! Didn't realize you'd accepted my suggestions because you pushed commits instead of accepting and applying in the browser.

thanks very much!

@jameslamb jameslamb merged commit 6dbe736 into microsoft:master Jan 22, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants