8000 Fix #4883 improved compatibility of FeatureHasher with dict-vectorizer by ryadzenine · Pull Request #4913 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Fix #4883 improved compatibility of FeatureHasher with dict-vectorizer #4913

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
Closed
wants to merge 1 commit into from
Closed

Fix #4883 improved compatibility of FeatureHasher with dict-vectorizer #4913

wants to merge 1 commit into from

Conversation

ryadzenine
Copy link

Hopefully, this one will be ok. Sorry for all the trouble guys and thank you for your help.

When a string value is encountered, is it concatenated with the feature name and we put a one on the relevant column.

@r-zenine
Fix scikit-learn#4883 improved compatibility of FeatureHasher with di…
< 8000 div class="js-socket-channel js-updatable-content" data-channel="eyJjIjoicmVwbzo4NDMyMjI6Y29tbWl0OjVmODM5MGM2YmE4MDk5YWE3MGFjZGY3MTNiNzJlYjdlM2M5ZDUwMmYiLCJ0IjoxNzQ3OTM4NjQyfQ==--5f2e6246f1fb693a62561f8fa528a1544e515d6b7930a65df76811709ad7b62d" data-url="/scikit-learn/scikit-learn/pull/4913/partials/commit_status_icon?oid=5f8390c6ba8099aa70acdf713b72eb7e3c9d502f">
5f8390c 
…ct-vectorizer
@amueller
Copy link
Member
amueller commented Jul 1, 2015

test are failing. you probably need to use six. also ping @ogrisel. I thought this was already supported, maybe I'm mistaken.

@ryadzenine
Copy link
Author

All the tests under feature_extraction are working on my machine. I can't figure out which one is failing.
Plus what is six ?

@amueller
Copy link
Member

You can click on the travis details to see the failures: https://travis-ci.org/scikit-learn/scikit-learn/jobs/69054099

@amueller
Copy link
Member

Six is a library to make code python2 and python3 compatible

@amueller
Copy link
Member

How did you run the tests? There are several failures under different versions: https://travis-ci.org/scikit-learn/scikit-learn/jobs/69054101

@ryadzenine
Copy link
Author

i just ran nosetests under feature_extraction

@amueller
Copy link
Member

Linking to #4883.

@amueller
Copy link
Member

I don't remember where, but @jnothman brought up that string features should be one-hot encoded. I'm not sure what the best way to handle categorical integer types is, though.

@rrohan
Copy link
Contributor
rrohan commented Oct 2, 2015

@amueller I am new to the scikit-learn community and want to contribute. Can I pick this up ?

@jnothman
Copy link
Member
jnothman commented Oct 6, 2015

Tests are failing because you forgot to commit the latest generated C code (i.e. it is inconsistent with the pyx code). Once that's sorted, if tests pass this almost LGTM, so @ryadzenine you're welcome to wrap it up or pass it onto @rrohan.

jnothman
jnothman reviewed Oct 6, 2015
View reviewed changes
sklearn/feature_extraction/_hashing.pyx
@@ -26,7 +26,6 @@ def transform(raw_X, Py_ssize_t n_features, dtype):

"""
assert n_features > 0

cdef np.int32_t h
cdef double value
Copy link
Member

Choose a reason for hiding this comment

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

Is this no longer used?

@raghavrv
Copy link
Member

@ryadzenine Could you confirm if you are still working on this or would you be okay if @rrohan picked this up?

@rrohan
Copy link
Contributor
rrohan commented Dec 8, 2015

@rvraghav93 Can I pick it up ?

@raghavrv
Copy link
Member
raghavrv commented Dec 8, 2015

Yes I guess... As we have waited for a week. But you might want to cherry-pick his commits and attribute him for the work as well if you don't mind :)

@devashishd12 devashishd12 mentioned this pull request Jan 15, 2016
Merged
@devashishd12
Copy link
Contributor

Sorry for interrupting your work @rrohan if you were working on this but I felt this needed quick addressing. Also thanks for the brilliant work @ryadzenine! @jnothman can you please check once?

@rrohan
Copy link
Contributor
rrohan commented Jan 15, 2016

Sorry, got held up. I will fix this over the weekend.

[Update] Just looked at your commit. Looks good :)

On Fri, Jan 15, 2016 at 8:25 AM, Devashish Deshpande <
notifications@github.com> wrote:

Sorry for interrupting your work @rrohan https://github.com/rrohan if
you were working on this but I felt this needed quick addressing. Also
thanks for the brilliant work @ryadzenine https://github.com/ryadzenine!
@jnothman https://github.com/jnothman can you please check once?

—
Reply to this email directly or view it on GitHub
#4913 (comment)
.

Rohan Ramanath

@ryadzenine
Copy link
Author

Hi guys, I am really sorry. I just disappeared. I had some personal issues to handle. Sorry!

@jnothman
Copy link
Member
jnothman commented Feb 1, 2016

We know what it's like to have lives outside of github. No problem. Does
that mean you'll get back to this now?

On 2 February 2016 at 09:33, Ryad ZENINE notifications@github.com wrote:

Hi guys, I am really sorry. I just disappeared. I had some personal issues
to handle. Sorry!

—
Reply to this email directly or view it on GitHub
#4913 (comment)
.

@ryadzenine
Copy link
Author

@jnothman No, I can't right now. Really sorry

@rrohan
Copy link
Contributor
rrohan commented Feb 3, 2016

I can work on it.
Should I ?

On Wed, Feb 3, 2016 at 12:27 PM, Ryad ZENINE notifications@github.com
wrote:

@jnothman https://github.com/jnothman No, I can't right now. Really
sorry

—
Reply to this email directly or view it on GitHub
#4913 (comment)
.

Rohan Ramanath

@amueller
Copy link
Member
amueller commented Oct 7, 2016

@rrohan are you still interested? This is up for grabs.

@rrohan
Copy link
Contributor
rrohan commented Oct 7, 2016

Sure, I can do it

On Fri, Oct 7, 2016 at 3:52 PM, Andreas Mueller notifications@github.com
wrote:

@rrohan https://github.com/rrohan are you still interested? This is up
for grabs.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4913 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFQe1iA1G1S3-M7TIne8vNsUfXtwA8YSks5qxs0agaJpZM4FPXmi
.

Rohan Ramanath

@rth
Copy link
Member
rth commented Jun 14, 2019

The underlying issue was resolved in #6173. Closing. Thanks!

@rth rth closed this Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ryadzenine @amueller @rrohan @jnothman @raghavrv @devashishd12 @rth @r-zenine
0