-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[tests][python] Make test that checks original pandas data isn't modified more strict #5267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! I left one suggestion for your consideration, but marking "approve" because I'm fine with whichever you decide about it.
@@ -654,16 +654,18 @@ def test_no_copy_when_single_float_dtype_dataframe(dtype, feature_name): | |||
assert np.shares_memory(X, built_data) | |||
|
|||
|
|||
@pytest.mark.parametrize('feature_name', [['x1'], 'auto']) | |||
@pytest.mark.parametrize('feature_name', [[42], 'auto']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.mark.parametrize('feature_name', [[42], 'auto']) | |
@pytest.mark.parametrize('feature_name', [['x1'], [42], 'auto']) |
Instead of replacing a string with a number here, would you consider adding a case here? That way, this test would still cover the case "feature_name
provided, and names are strings".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proposal! Added in 2b107f6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes look great, thank you!
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. |
Sorry, should have proposed that in #5254.
Check that
_data_from_pandas()
doesn't modify column names (doesn't convert int to string) of original data.Refer to #5254 (review).