-
Notifications
You must be signed in to change notification settings - Fork 86
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
Added string representation to AutoBase #675
Conversation
Codecov Report
@@ Coverage Diff @@
## master #675 +/- ##
==========================================
+ Coverage 99.29% 99.32% +0.02%
==========================================
Files 140 140
Lines 4981 5018 +37
==========================================
+ Hits 4946 4984 +38
+ Misses 35 34 -1
Continue to review full report at Codecov.
|
Notes:
|
9a78d24
to
4b230e4
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.
Looking good! Some comments and needs more test coverage. Maybe removing the uncalled function will do the trick.
docs/source/changelog.rst
Outdated
@@ -34,6 +34,7 @@ Changelog | |||
* Changed requirements-parser to be a core dependency :pr:`673` | |||
* Replace `supported_problem_types` field on pipelines with `problem_type` attribute on base classes :pr:`678` | |||
* Update `ModelFamily` values: don't list xgboost/catboost as classifiers now that we have regression pipelines for them :pr:`677` | |||
* Added __repr__ for AutoSearch object :pr:`675` |
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.
__str__
😄
evalml/automl/auto_base.py
Outdated
@@ -100,6 +100,63 @@ def __init__(self, problem_type, tuner, cv, objective, max_pipelines, max_time, | |||
logger.log("Warning: unable to import plotly; skipping pipeline search plotting\n") | |||
self.plot = None | |||
|
|||
def __str__(self): | |||
def _pipeline_names(): |
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.
Kind of confused how this is different from _allowed_model_families()
or how they are named. Is this for pipelines? or is _allowed_model_families
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.
Actually seems like this isn't used.
evalml/automl/auto_base.py
Outdated
all_objectives += '\n\t' | ||
return all_objectives[:-2] | ||
|
||
def _allowed_model_families(): |
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.
Should this just be Possible Pipelines
? Same thing in searc_desc
56565df
to
e4cc0e0
Compare
evalml/automl/auto_base.py
Outdated
all_objectives += objective.name | ||
all_objectives += '\n\t' | ||
return all_objectives[:-2] | ||
|
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.
Hmmmm... I think this could be simplified? To:
def _obj_names():
all_objectives = ""
for objective in self.additional_objectives:
all_objectives += "\n\t" + objective.name
return all_objectives
Or I think using .join
could also work here (with some check for last element)?
evalml/automl/auto_base.py
Outdated
all_objectives += '\n\t' | ||
return all_objectives[:-2] | ||
|
||
def _possible_pipelines(): |
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.
Same comment as for _obj_names
:D
|
||
automl.search(X, y, raise_errors=False) | ||
str_rep = str(automl) | ||
# str_rep = str_rep.lower() |
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.
Leftover comment here :o?
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! I just left a comment about simplifying some functions, but otherwise LGTM :)
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.
LGTM if you address Angela's comments!
Lint fixes
dd1ecd9
to
da93f2a
Compare
f"Detect Label Leakage: {self.detect_label_leakage}\n" | ||
f"Start Iteration Callback: {_get_funct_name(self.start_iteration_callback)}\n" | ||
f"Add Result Callback: {_get_funct_name(self.add_result_callback)}\n" | ||
f"Additional Objectives: {_print_list(self.additional_objectives)}\n" |
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.
What will this print out?
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.
f"Objective: {self.objective.name}\n" | ||
f"Max Time: {self.max_time}\n" | ||
f"Max Pipelines: {self.max_pipelines}\n" | ||
f"Possible Pipelines: {_print_list(self.possible_pipelines)}\n" |
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.
What will this print out?
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.
try: | ||
rankings_str = self.rankings.drop(['parameters'], axis='columns').to_string() | ||
rankings_desc = f"\nSearch Results: \n{'='*20}\n{rankings_str}" | ||
except KeyError: |
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.
@christopherbunn when would this happen? And is there any way to restructure this code to avoid having a try
/except
? Its nice to avoid these whenever possible because they make the flow harder to follow.
Could we do something like this:
rankings = self.rankings
if 'parameters' not in rankings.columns:
return search_desc
return search_desc + "\nSearch Results: \n{'='*20}\n{rankings_str}".format(rankings['parameters'])
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.
@dsherry The goal of this snippet was to only include the rankings after the search process has been ran. The reason I had the try-catch is that rankings is a @property
and this KeyError is what I get when I try to call automl.rankings
before any searches is ran. Is there a better way to check if the search process has ran yet?
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.
@christopherbunn nice find. Could you please file this as a bug? :)
I suppose we could add a @property
which is something like AutoBase.has_run
. That way we could have whatever implementation we want for that method. Not critical though.
|
||
|
||
@patch('evalml.pipelines.BinaryClassificationPipeline.fit') | ||
def test_automl_str_search(mock_fit, X_y): |
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.
@christopherbunn are there any edge cases which this test doesn't currently cover? For instance, is there any way a None
value could be provided as the input to one of the fields you apply your _print_list
and _get_funct_name
helpers to?
Before search is ran:
After search is ran:
Resolves #481