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.

Sign up for GitHub

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

Jump to bottom

[tests] replace pytest.parametrize #4377

Merged
merged 2 commits into from
Jun 15, 2021
Merged

[tests] replace pytest.parametrize #4377

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7dda081
replace pytest.parametrize
StrikerRUS Jun 14, 2021
4445482
add informative message for assert
StrikerRUS Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions tests/python_package_test/test_engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,8 +1252,8 @@ def generate_trainset_for_monotone_constraints_tests(x3_to_category=True):
return trainset


@pytest.mark.parametrize("test_with_interaction_constraints", [True, False])
def test_monotone_constraints(test_with_interaction_constraints):
@pytest.mark.parametrize("test_with_categorical_variable", [True, False])
def test_monotone_constraints(test_with_categorical_variable):
def is_increasing(y):
return (np.diff(y) >= 0.0).all()

Expand Down Expand Up @@ -1316,10 +1316,12 @@ def has_interaction(treef):

return not has_interaction_flag.any()

for test_with_categorical_variable in [True, False]:
trainset = generate_trainset_for_monotone_constraints_tests(
test_with_categorical_variable
)
trainset = generate_trainset_for_monotone_constraints_tests(
test_with_categorical_variable
)
for test_with_interaction_constraints in [True, False]:
Copy link
Collaborator

@jameslamb jameslamb Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm ok with this change, but could you update the assert on line 1334 with an error message that will tell us which case it failed on if it fails?

error_msg = f"Model not correctly constrained (test_with_interaction_constraints={test_with_interaction_constraints})"

assert is_correctly_constrained(
    constrained_model, test_with_categorical_variable
), error_msg

Without that, that particular test could fail and we wouldn't know which case it failed on just from the logs.

I think it would also be useful to include the value of monotone_constraints_method in that message too (since it is similarly from an inner for loop), but that's not directly related to this PR so up to you if you want to do that at the same time or separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Done in 4445482.

error_msg = ("Model not correctly constrained "
f"(test_with_interaction_constraints={test_with_interaction_constraints})")
for monotone_constraints_method in ["basic", "intermediate", "advanced"]:
params = {
"min_data": 20,
Expand All @@ -1333,7 +1335,7 @@ def has_interaction(treef):
constrained_model = lgb.train(params, trainset)
assert is_correctly_constrained(
constrained_model, test_with_categorical_variable
)
), error_msg
if test_with_interaction_constraints:
feature_sets = [["Column_0"], ["Column_1"], "Column_2"]
assert are_interactions_enforced(constrained_model, feature_sets)
Expand Down Expand Up @@ -1399,8 +1401,9 @@ def test_monotone_penalty_max():
}

unconstrained_model = lgb.train(params_unconstrained_model, trainset_unconstrained_model, 10)
unconstrained_model_predictions = unconstrained_model.\
predict(x3_negatively_correlated_with_y.reshape(-1, 1))
unconstrained_model_predictions = unconstrained_model.predict(
x3_negatively_correlated_with_y.reshape(-1, 1)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this formatting a lot more, thank you


for monotone_constraints_method in ["basic", "intermediate", "advanced"]:
params_constrained_model["monotone_constraints_method"] = monotone_constraints_method
Expand Down