-
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
Remove copy_dataframe
parameter, update requirements with release of Woodwork v0.0.6
#1478
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1478 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 223 223
Lines 15024 15025 +1
=======================================
+ Hits 15017 15018 +1
Misses 7 7
Continue to review full report at Codecov.
|
@@ -1776,7 +1776,7 @@ def test_automl_woodwork_user_types_preserved(mock_binary_fit, mock_binary_score | |||
assert arg.semantic_tags['cat col'] == {'category'} | |||
assert arg.logical_types['cat col'] == ww.logical_types.Categorical | |||
assert arg.semantic_tags['num col'] == {'numeric'} | |||
assert arg.logical_types['num col'] == ww.logical_types.WholeNumber |
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.
WholeNumber was removed
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.
@angela97lin Thanks for the fix!
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.
Thanks @angela97lin !
One question: why didn't our dependency update bot catch this? Shouldn't we have one or more unit tests which should have failed with the removal of WholeNumber
and/or copy_dataframe
?
if isinstance(ww_data, pd.Series): | ||
return ww.DataColumn(ww_data) | ||
return ww.DataTable(ww_data, copy_dataframe=True) | ||
|
||
return ww.DataTable(ww_data) |
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.
Hmm, so is DataTable
copying implicitly now? The docs for the constructor just say "create datatable"
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.
EDIT: Oops, didn't read your comment thoroughly. It doesn't copies implicitly.
See description for alteryx/woodwork#398!
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.
Thanks @angela97lin ! Filed alteryx/woodwork#420
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!
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.
very cool!
@dsherry Not sure why our dependency bot didn't catch this... but our unit tests didn't fail because the last merge happened before woodwork released. Just reran tests on main and they break, as expected: https://app.circleci.com/pipelines/github/alteryx/evalml/7581/workflows/c7130efe-954f-4c2a-a8eb-5e29a90eeedc/jobs/85802 |
@angela97lin oh I know why... we don't have |
@dsherry Ah, for some reason I thought the list was directly taken from |
copy_dataframe
parameter which was removed in 0.0.6