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

removing deprecated mean_squared_error, cv_folds_file and objectives in learning curve #470

Merged

Conversation

ghost
Copy link

@ghost ghost commented Feb 22, 2019

This PR removes the support for mean_squared_error, cv_folds_file and objectives in learning curve.

mean_squared_error is replaced by neg_mean_squared_error
cv_folds_file is replaced by folds_file
objectives in learning curve is replaced by metrics in output section

@coveralls
Copy link

coveralls commented Feb 22, 2019

Coverage Status

Coverage decreased (-0.02%) to 93.93% when pulling 1720f0b on feature/deprecate-things-with-deprecation-warnings into d28116f on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 93.436% when pulling 0895907 on feature/deprecate-things-with-deprecation-warnings into 5481d0c on master.

skll/config.py Show resolved Hide resolved
skll/config.py Show resolved Hide resolved
tests/test_input.py Outdated Show resolved Hide resolved
tests/test_input.py Show resolved Hide resolved
@@ -1751,7 +1789,7 @@ def test_learning_curve_objectives_and_no_metrics():
eq_(grid_objectives, [])


def test_learning_curve_default_objectives_and_no_metrics():
def test_learning_curve_no_metrics():
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this test raise an error? Is there a default learning curve metric?

Copy link
Author

Choose a reason for hiding this comment

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

This test is calling check_config_parsing_value_error which raises ValueError. There is no default metrics. Instead, I can change this test case to call _parse_config_file and raise exception if you think that would be better.

@desilinguist desilinguist self-requested a review March 14, 2019 15:15
Copy link
Member

@desilinguist desilinguist left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my previous comments. There seems to be a bug that removes yield from one of the tests and I am not sure about one of the changes you made. We are pretty close.

tests/test_input.py Outdated Show resolved Hide resolved
tests/test_output.py Show resolved Hide resolved
@desilinguist
Copy link
Member

desilinguist commented Mar 14, 2019

Do we really need these two lines if we are raising a ValueError anyway? I think that's also why the coverage is going down.

Copy link
Contributor

@mulhod mulhod left a comment

Choose a reason for hiding this comment

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

LGTM

@desilinguist desilinguist merged commit 90b6098 into master Mar 14, 2019
@desilinguist desilinguist deleted the feature/deprecate-things-with-deprecation-warnings branch March 14, 2019 19:12
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.

None yet

3 participants