Skip to content
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

Quick fix to retain typing #2461

Merged
merged 11 commits into from Jul 19, 2021
Merged

Quick fix to retain typing #2461

merged 11 commits into from Jul 19, 2021

Conversation

chukarsten
Copy link
Collaborator

Addresses: #2456

This modifies the for-loop to handle Ordinal and Datetime ltypes to pass through the properties that were missing. The other ltypes are still handled the same way. When Woodwork addresses this, we can revert this change.

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #2461 (b3b8543) into main (019de6e) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2461     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        283     283             
  Lines      25709   25730     +21     
=======================================
+ Hits       25608   25629     +21     
  Misses       101     101             
Impacted Files Coverage Δ
evalml/tests/utils_tests/test_woodwork_utils.py 100.0% <100.0%> (ø)
evalml/utils/woodwork_utils.py 100.0% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 019de6e...b3b8543. Read the comment docs.

@dsherry
Copy link
Contributor

dsherry commented Jul 1, 2021

@chukarsten did you mean to say "Fixes #2456 " in the description above?

@chukarsten chukarsten changed the title Quick fix to retain typing WIP: Quick fix to retain typing Jul 7, 2021
@chukarsten chukarsten marked this pull request as draft July 7, 2021 20:23
@chukarsten chukarsten changed the title WIP: Quick fix to retain typing Quick fix to retain typing Jul 7, 2021
@@ -110,13 +111,20 @@ def _retain_custom_types_and_initalize_woodwork(
return ww.init_series(new_dataframe, old_logical_types)
if ltypes_to_ignore is None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@freddyaboulton do we have a plan for phasing out _retain_custom_types_and_initialize_woodwork? I know your PR dropped some usages of it. If we need to file an issue with WW to move some functionality over, I'll do it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chukarsten I think we need it for the time being for the Imputer because the imputer will "break" the schema and we need a way of recreating it while allowing some of the types to change. I've been told that alteryx/woodwork#965 will help with that but I think we need to think about that further.

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for doing this! Do we have an issue tracking removing this code once WW makes their changes? 😮

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me @chukarsten !

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left a question, but nothing blocking. Would be nice to file an issue to track removing this once WW fixes!

Addressed comments about user defined order and dt format checking.
@chukarsten chukarsten merged commit 9145715 into main Jul 19, 2021
@chukarsten chukarsten deleted the woodwork_retain_types branch July 19, 2021 19:00
@chukarsten chukarsten mentioned this pull request Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants