Thank you for the PR @ly648499246 !

Can you add a non-regression test that would fail at master but pass in this PR?

Thank you for your attention.
The case is

from sklearn.feature_extraction.text import HashingVectorizer

hashing = HashingVectorizer(n_features=1000000, ngram_range=(2,3), strip_accents='ascii')

print(hashing.transform(['22pcs efuture']).indices)

before this change , this code will print array([-483648], dtype=int32)
after this change, this code will print array([483648], dtype=int32)

And we can also test this case:

from sklearn.feature_extraction.text import HashingVectorizer

hashing = HashingVectorizer(n_features=1000000, ngram_range=(2,3), strip_accents='ascii')

print(hashing.transform(['22pcs efuture']))

before this change , this code will throw a exception: ValueError: negative column index found
after this change, this code will print (0, 483648) -1.0

This case works in Linux and Windows.
Results of different platform are as follow.

Would the hash value for all input change then? Which is probably fine in this case as a bug fix, but we need to add a note on breaking backward compatibility (and potentially the token collisions that happen) in the release notes.

Please add an entry to the change log at doc/whats_new/v1.0.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself with :user:.

For the non-regression test, we could monkey patch mumurhash function to ensure to return -2 ** 31 without having to run a potential expensive hashing fit?

Would the hash value for all input change then? Which is probably fine in this case as a bug fix, but we need to add a note on breaking backward compatibility (and potentially the token collisions that happen) in the release notes.

Please add an entry to the change log at doc/whats_new/v1.0.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself with :user:.

Thank you for your advice, I already do some test locally, hash value for other value would not change.
And I will add entry to the change log.

For the non-regression test, we could monkey patch mumurhash function to ensure to return -2 ** 31 without having to run a potential expensive hashing fit?

Thanks for your advice!

Maybe I'm ignorant, Is there a way to monkey patch mumurhash function? Because _hashing_fast and murmurhash is precompiled.

And in my case above, I can promise that the mumurhash function will return -2**31 because I print it to see when I test.

Moreover, the test above is without fit, it is fast to run, maybe you can try it.

It‘s really a latent bug, nice work

Thank you for your advice @jeremiedbb !
This is a more safe change and I committed the suggestion.

For the non-regression test, we could monkey patch mumurhash function to ensure to return -2 ** 31 without having to run a potential expensive hashing fit?

@glemaitre it's only hashing 1 string. The 100000 features only specify the range for the indices but the resulting array is a sparse matrix containing only 1 non-zero element.

@glemaitre it's only hashing 1 string. The 100000 features only specify the range for the indices but the resulting array is a sparse matrix containing only 1 non-zero element.

OK so it should not be an issue then.

thanks @ly648499246. Just nitpicks

Thanks @jeremiedbb for your careful check!

Thank you for working on this @ly648499246 !