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
Fix Automl custom objective stack trace #1575
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1575 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 240 240
Lines 18210 18235 +25
=========================================
+ Hits 18202 18227 +25
Misses 8 8
Continue to review full report at Codecov.
|
ff1fbba
to
e4ea401
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.
Looks good! Left one question, but nothing blocking
@@ -619,15 +619,17 @@ def _add_baseline_pipelines(self, X, y): | |||
return False | |||
|
|||
@staticmethod | |||
def _get_mean_cv_scores_for_all_objectives(cv_data): | |||
def _get_mean_cv_scores_for_all_objectives(cv_data, objective_name_to_class): |
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.
Could you just call self.objective_name_to_class
inside this function rather than pass it in as an arg? Is there a reason to choose one versus the other, since the objectives don't change post-initialization?
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.
Wanted to keep it static 🤷♂️ Yes you're right though. Functionally, there's no difference between this implementation and just adding self
as an argument to the method.
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.
Got it, sounds good!
@@ -34,7 +34,7 @@ class MockNoObjectiveFunctionObjective(ObjectiveBase): | |||
MockNoObjectiveFunctionObjective() | |||
|
|||
|
|||
@pytest.mark.parametrize("obj", _get_subclasses(ObjectiveBase)) | |||
@pytest.mark.parametrize("obj", _all_objectives_dict().values()) |
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!
99743dc
to
fa38e71
Compare
771272b
to
0c3ce80
Compare
…names_of_defined_objectives
0491c59
to
2242ffd
Compare
Pull Request Description
Fixes #1572
The issue is that
_compute_cv_scores
stores the objective scores in a dict keyed by the objective name (str). When we go to compute "percent better than baseline", we have to map the string name to an objective class. We were doing this mapping withget_objective
which doesn't doesn't know about the class definition of a custom objective.The fix is to create a name to class mapping when AutoMLSearch is created.
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
.