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
Adding positive_only objectives to non_core_objectives #1661
Conversation
assert all([obj not in only_positive for obj in search.additional_objectives]) | ||
|
||
|
||
def test_automl_validate_objective(X_y_regression): |
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.
I don't think we had coverage for this before? Let me know if this is redundant!
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.
@freddyaboulton Good addition, but why specifically for these 2 objectives?
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.
Just cause they were non-core but I changed the test to check all non-core objectives hehe!
Codecov Report
@@ Coverage Diff @@
## main #1661 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 240 240
Lines 18485 18503 +18
=========================================
+ Hits 18477 18495 +18
Misses 8 8
Continue to review full report at Codecov.
|
33fb266
to
ea9743b
Compare
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.
Nice!
|
||
@pytest.mark.parametrize('problem_type', [ProblemTypes.BINARY, ProblemTypes.MULTICLASS, | ||
ProblemTypes.TIME_SERIES_REGRESSION, ProblemTypes.REGRESSION]) | ||
def test_automl_does_not_include_positive_only_objectives_by_default(problem_type, X_y_regression): |
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.
Nice!
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.
Looks great!
assert all([obj not in only_positive for obj in search.additional_objectives]) | ||
|
||
|
||
def test_automl_validate_objective(X_y_regression): |
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.
@freddyaboulton Good addition, but why specifically for these 2 objectives?
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.
looks good!
Pull Request Description
Fixes #1660
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.