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.

Sign up for GitHub

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

Jump to bottom

Added basic one-hot encoding #73

Merged
merged 19 commits into from
Sep 18, 2019
Merged

Added basic one-hot encoding #73

merged 19 commits into from
Sep 18, 2019

Conversation

jeremyliweishih
Copy link
Collaborator

No description provided.

@jeremyliweishih jeremyliweishih requested review from kmax12 and removed request for kmax12 September 13, 2019 18:58
@jeremyliweishih jeremyliweishih changed the title Added basic one-hot encoding WIP: Added basic one-hot encoding Sep 13, 2019
@jeremyliweishih jeremyliweishih changed the title WIP: Added basic one-hot encoding Added basic one-hot encoding Sep 13, 2019
@jeremyliweishih jeremyliweishih added this to the Sprint #1 milestone Sep 13, 2019
kmax12
kmax12 suggested changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@kmax12 kmax12 left a comment

Choose a reason for hiding this comment

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

overall looks good. one thing we should add in is a test for looking at variable importance after doing the categorical encoding.

we might also look for somewhere in the documentation to add a comment about handling categoricals now

evalml/pipelines/classification/logistic_regression.py Outdated Show resolved Hide resolved
evalml/pipelines/classification/logistic_regression.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
kmax12
kmax12 suggested changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@kmax12 kmax12 left a comment

Choose a reason for hiding this comment

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

just one comment. otherwise, just need to add documentation and update fraud demo

requirements.txt Outdated Show resolved Hide resolved
kmax12
kmax12 previously approved these changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@kmax12 kmax12 left a comment

Choose a reason for hiding this comment

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

This looks good, but we still need to the documentation updates. Could be in this PR or another, whichever you prefer

kmax12
kmax12 suggested changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@kmax12 kmax12 left a comment

Choose a reason for hiding this comment

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

just one comment, otherwise good to merge

docs/source/roadmap.rst Outdated Show resolved Hide resolved
kmax12
kmax12 approved these changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@kmax12 kmax12 left a comment

Choose a reason for hiding this comment

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

LGTM

@jeremyliweishih jeremyliweishih merged commit a3d3304 into master Sep 18, 2019
@jeremyliweishih jeremyliweishih deleted the one-hot branch September 18, 2019 15:10
@angela97lin angela97lin mentioned this pull request Oct 29, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmax12 kmax12 kmax12 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Sprint #1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jeremyliweishih @kmax12