-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Error handling for invalid dataset type when creating models. #2328
Error handling for invalid dataset type when creating models. #2328
Conversation
@@ -90,6 +90,7 @@ def _find_only_column_of_type(sframe, target_type, type_name, col_name): | |||
raised. The name and type of the target column should be provided as | |||
strings for the purpose of error feedback. | |||
""" | |||
_raise_error_if_not_sframe(sframe,"dataset") |
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.
If the sframe
variable is not an SFrame, this will raise an error message claiming the name of the input parameter was "dataset". I don't think we want to assume the input parameter will always be name "dataset".
Sadly, I think we'll need to add this check to each .create
method that doesn't already have it.
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 fixed all files which didn't check the type error.
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.
This is a great change. Thanks for adding it.
@@ -220,6 +220,8 @@ def create(dataset, target, feature=None, model = 'resnet-50', | |||
ImageClassifier | |||
""" | |||
start_time = _time.time() | |||
if not isinstance(dataset,_tc.SFrame): |
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.
Nit: you should have a space after the comma.
@@ -101,6 +101,8 @@ def create(dataset, label = None, feature = None, model = 'resnet-50', verbose = | |||
[500 rows x 4 columns] | |||
""" | |||
start_time = _time.time() | |||
if not isinstance(dataset,_tc.SFrame): |
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.
Nit: same as previous comment.
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 fixed it
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.
Here too use https://help.github.com/en/articles/closing-issues-using-keywords to refer the #1978, and give the PR and commit message more descriptive text
Just add one line to check the datatype for sframe,
since some codes don't check datatype for the dataset before passing it into _find_only_column_of_type()
Close #1978