Skip to content

ENH Improving coverage of MinHashEncoder - #379

Merged
LilianBoulard merged 12 commits into
skrub-data:mainfrom
jovan-stojanovic:improve_min_hash_encoder_coverage
Oct 25, 2022
Merged

LilianBoulard merged 12 commits into
skrub-data:mainfrom
jovan-stojanovic:improve_min_hash_encoder_coverage

Conversation

@jovan-stojanovic

@jovan-stojanovic jovan-stojanovic commented Oct 7, 2022 •

Copy link
Copy Markdown
Member

Fix #371 and part of #376

I replaced the data that needed to be downloaded in the tests with generated ones.

Coverage is up to 98%. Only the _more_tags function is not covered (as in #375)..

Had also to take into account empty strings, which was not the case previously. Linked to #378
Moved up the parameters value verifications to the .fit() method, which seems more logical.

@jovan-stojanovic jovan-stojanovic changed the title Improving coverage and other small issues Improving coverage of MinHashEncoder and other small issues Oct 7, 2022
@jovan-stojanovic jovan-stojanovic changed the title Improving coverage of MinHashEncoder and other small issues Improving coverage of MinHashEncoder Oct 7, 2022
Comment thread dirty_cat/_minhash_encoder.py Outdated
Comment thread dirty_cat/_minhash_encoder.py
Comment thread dirty_cat/tests/test_minhash_encoder.py Outdated
@GaelVaroquaux

GaelVaroquaux commented Oct 17, 2022 via email

Copy link
Copy Markdown
Member

@LilianBoulard

LilianBoulard commented Oct 17, 2022 •

Copy link
Copy Markdown
Member

I don't think that it applies, as it's in a private module.

It does apply because minhash is an instance method of the MinHashEncoder, which is part of the public API.

@jovan-stojanovic

jovan-stojanovic commented Oct 20, 2022 •

Copy link
Copy Markdown
Member Author

Ok, after some investigation, I think that there is no need to make it private.
Actually, this method is the same as get_fast_hash but using the murmur method. So I renamed it accordingly (as get_murmur_hash) and it can be used as the other methods.. that is applied to any string after fitting the encoder.

@GaelVaroquaux

Copy link
Copy Markdown
Member

This branch has mixed history and the diff is very hard to read.

Maybe you could cherry-pick the relevant commits (potentially in a new branch, if needed)

https://www.atlassian.com/git/tutorials/cherry-pick

@jovan-stojanovic
jovan-stojanovic force-pushed the improve_min_hash_encoder_coverage branch from 49b5960 to be8f371 Compare October 24, 2022 08:51
@jovan-stojanovic

Copy link
Copy Markdown
Member Author

This branch has mixed history and the diff is very hard to read.

Maybe you could cherry-pick the relevant commits (potentially in a new branch, if needed)

https://www.atlassian.com/git/tutorials/cherry-pick

Sorry, this was due to the renaming from master to main. It's back to normal now.

Comment thread dirty_cat/_minhash_encoder.py Outdated
Comment thread dirty_cat/_minhash_encoder.py
Comment thread dirty_cat/tests/test_docstrings.py Outdated
@GaelVaroquaux

GaelVaroquaux commented Oct 24, 2022 via email

Copy link
Copy Markdown
Member

@LilianBoulard

Copy link
Copy Markdown
Member

Sure, LGTM so here we go :)

@LilianBoulard
LilianBoulard merged commit e78763c into skrub-data:main Oct 25, 2022
@jovan-stojanovic jovan-stojanovic changed the title Improving coverage of MinHashEncoder ENH Improving coverage of MinHashEncoder Nov 16, 2022
@jovan-stojanovic
jovan-stojanovic deleted the improve_min_hash_encoder_coverage branch November 17, 2022 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve coverage of main features

3 participants