Skip to content

Have OHE return Boolean columns - #2170

Merged
bchen1116 merged 10 commits into
mainfrom
bc_2169_ohe
Apr 22, 2021
Merged

Have OHE return Boolean columns#2170
bchen1116 merged 10 commits into
mainfrom
bc_2169_ohe

Conversation

@bchen1116

@bchen1116 bchen1116 commented Apr 21, 2021

Copy link
Copy Markdown
Contributor

fix #2169

@bchen1116 bchen1116 self-assigned this Apr 21, 2021
@codecov

codecov Bot commented Apr 21, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2170 (bb017db) into main (3caee9e) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main    #2170     +/-   ##
=========================================
+ Coverage   100.0%   100.0%   +0.1%     
=========================================
  Files         295      295             
  Lines       24376    24389     +13     
=========================================
+ Hits        24366    24379     +13     
  Misses         10       10
Impacted Files Coverage Δ
...components/transformers/encoders/onehot_encoder.py 100.0% <100.0%> (ø)
...alml/tests/component_tests/test_one_hot_encoder.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 3caee9e...bb017db. Read the comment docs.

jeremyliweishih
jeremyliweishih approved these changes Apr 22, 2021
View reviewed changes

@jeremyliweishih jeremyliweishih left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this looks great Bryan!

Comment thread evalml/tests/pipeline_tests/test_pipelines.py Outdated
freddyaboulton
freddyaboulton approved these changes Apr 22, 2021
View reviewed changes

@freddyaboulton freddyaboulton left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good @bchen1116 !!

dsherry
dsherry approved these changes Apr 22, 2021
View reviewed changes
Comment thread evalml/tests/component_tests/test_one_hot_encoder.py
if name == 'integers':
assert str(types) == "Integer"
else:
assert str(types) == "Boolean"

@dsherry dsherry Apr 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great! I love that you made this general and not specific to xgboost.

One check I'd add: assert that the right number of features has been generated. That way, if for some reason the OHE isn't generating any features at all for the bool/category inputs, the test will fail.

@bchen1116
bchen1116 merged commit 1762f83 into main Apr 22, 2021
This was referenced May 4, 2021
Closed
Merged
@freddyaboulton
freddyaboulton deleted the bc_2169_ohe branch May 13, 2022 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeremyliweishih jeremyliweishih jeremyliweishih approved these changes
@angela97lin angela97lin Awaiting requested review from angela97lin
@chukarsten chukarsten Awaiting requested review from chukarsten
@ParthivNaresh ParthivNaresh Awaiting requested review from ParthivNaresh
+2 more reviewers
@dsherry dsherry dsherry approved these changes
@freddyaboulton freddyaboulton freddyaboulton approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@bchen1116 bchen1116

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Have OHE return Boolean columns

4 participants

@bchen1116 @dsherry @jeremyliweishih @freddyaboulton