-
Notifications
You must be signed in to change notification settings - Fork 907
Raise an error when DatetimeTimeIndex is a variable but time_index is empty #723
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #723 +/- ##
==========================================
+ Coverage 97.63% 97.63% +<.01%
==========================================
Files 118 118
Lines 10194 10205 +11
==========================================
+ Hits 9953 9964 +11
Misses 241 241
Continue to review full report at Codecov.
|
featuretools/entityset/entityset.py
Outdated
| raise ValueError("time_index and index cannot be the same value, %s" % (time_index)) | ||
|
|
||
| if vtypes.DatetimeTimeIndex in variable_types.values() and time_index is None: | ||
| raise ValueError("'DatetimeTimeIndex' must be set using time_index parameter") |
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.
We should include the variable name in the error message to help the user track down the source of the problem
featuretools/entityset/entityset.py
Outdated
| raise ValueError("time_index and index cannot be the same value, %s" % (time_index)) | ||
|
|
||
| if vtypes.DatetimeTimeIndex in variable_types.values() and time_index is None: | ||
| var_name = variable_types.keys()[variable_types.values().index(vtypes.DatetimeTimeIndex)] |
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 works but I think we can clean it up a bit
We can loop through variable_types.items() once and raise an error the first time we encounter the problem.
featuretools/entityset/entityset.py
Outdated
| if time_index is None: | ||
| for variable, variable_type in variable_types.items(): | ||
| if variable_type == vtypes.DatetimeTimeIndex: | ||
| raise ValueError("Variable %s must be set using time_index parameter" % (variable)) |
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.
Can you also include that the variable is DatetimeTimeIdex type, e.g. "DatetimeTimeIndex variable %s . . ."
rwedge
left a 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.
Looks good
Added a ValueError message when a DatetimeTimeIndex is set as a variable but is not specified as a time_index
fixes #497