Related: #2665

I think to be a strong test, you need to ensure that there's enough data to have expected a zero, or else have a test that can infer that a zero was produced, such as checking that 5 words produced 3 nonzero hashes (therefore a hash collision).

I suggest you use HashingVectorizer(analyzer='char', n_features=1). Then fit_transform(['ab', 'ac', 'ad', 'ae', 'af', ...]) should produce at least one row where a zero needs to be eliminated. The chance of this occurring is 1/2 ('a' being 1 and the other char being -1, or vice versa).

However, this is actually an issue in FeatureHasher, not HashingVectorizer, so you should really be testing that.

@jnothman Thanks for the feedback, and for pointing out that other issue. For a hash collision the chance of getting a 0 is 1/2, but the whole process is deterministic, so once we found one case, it should be reproducible as long as the hashing function does not change. I did test that the previous unit test fail before the PR and passed after it: i.e. "investigation" and "need records" produce a hash collision that consistently sums up as 0 (example taken from one of the parent issues). Nevertheless, I updated the unit tests following your suggestion as that is indeed probably more robust.

However, now I'm not convinced that using eliminate_zeros() is a good solution here (cf the issue linked above) so putting this PR on hold for the moment.

When I tried locally it didn't seem like the previous example created a collision with the default n_features. I got 5 nonzero entries for 3 unigrams and 2 bigrams.

Apart from time, in what way does #2655 argue against eliminate_zeros?

Well I did get a zero due to collision before that PR, in the unit test,

>>> HashingVectorizer(ngram_range=(1, 2), non_negative=True).fit_transform(['investigation need records']).data
array([ 0.57735027,  0.57735027,  0.        ,  0.57735027])

but then maybe some part of the processing is system dependent. Anyway, changed the unit tests to be more robust following your suggestions.

No I meant that addressing issue #7513 would probably a better way of solving this than using eliminate_zeros.

oh. I must have typed it in incorrectly. Yes, that's a fair test... as long
as you then assert that it meets your expectations of having fewer than 5
elements in data.

On 29 September 2016 at 18:09, Roman Yurchak notifications@github.com
wrote:

Well I did get a zero due to collision before that PR, in the unit test,

HashingVectorizer(ngram_range=(1, 2), non_negative=True).fit_transform(['investigation need records']).data
array([ 0.57735027, 0.57735027, 0. , 0.57735027])

but then maybe some part of the processing is system dependent. Anyway,
changed the unit tests to be more robust following your suggestions.

No I meant that addressing issue #7513
#7513 would probably
a better way of solving this than using eliminate_zeros.

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

Closing this PR as it's superseded by PR #7565.