Repository: dirty-cat/dirty_cat

Issue:
Improve coverage of main features
Here are some of the code coverage results:
``` 
------------------------------------------------------------------------
dirty_cat/_datetime_encoder.py             88%
dirty_cat/_gap_encoder.py                  88%
dirty_cat/_minhash_encoder.py              94%
dirty_cat/_similarity_encoder.py           90%
dirty_cat/_string_distances.py             91%
dirty_cat/_super_vectorizer.py             85%
dirty_cat/_target_encoder.py               78%
dirty_cat/_utils.py                        93%
dirty_cat/datasets/_fetching.py            97%
...
------------------------------------------------------------------------
TOTAL                                     89%
```
It may be good to work on improving those so that we have clear standards on what coverage is needed for new features.

### Tests to Update

- [ ] `dirty_cat._datetime_encoder.DatetimeEncoder`
- [ ] `dirty_cat._gap_encoder.GapEncoder`
- [ ] `dirty_cat/_minhash_encoder.py` #379 
- [ ] `dirty_cat/_similarity_encoder.py`
- [ ] `dirty_cat/_string_distances.py`
- [ ] `dirty_cat/_super_vectorizer.py`
- [ ] `dirty_cat/_target_encoder.py` #375 
- [ ] `dirty_cat/_utils.py`
- [ ] `dirty_cat/datasets/_fetching.py`

Discussion:
GaelVaroquaux: > In dirty_cat/_minhash_encoder.py:

> > -    def minhash(

> Given that this method is not private,

I don't think that it applies, as it's in a private module. Is it exposed in a public module (we renamed all the modules for this specific reason).

jovan-stojanovic: 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 and it can be used as the other methods.. that is applied to any string after fitting the encoder.
GaelVaroquaux: 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: > 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.
LilianBoulard: Sure, LGTM so here we go :)

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 96fbf2e..6b1ee03 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -18,10 +18,12 @@ Minor changes
 -------------
 * Removed example `Fitting scalable, non-linear models on data with dirty categories`.
 
+* :class:`MinHashEncoder`'s `minhash` method is no longer public.
+
 Bug fixes
 ---------
 
-* :class:`MinHashEncoder` now considers `None` as missing values, rather
+* :class:`MinHashEncoder` now considers `None` and empty strings as missing values, rather
   than raising an error.
 
 Release 0.3.0
diff --git a/dirty_cat/_minhash_encoder.py b/dirty_cat/_minhash_encoder.py
index 8c6c86c..5dc0233 100644
--- a/dirty_cat/_minhash_encoder.py
+++ b/dirty_cat/_minhash_encoder.py
@@ -28,6 +28,7 @@ from ._utils import LRUDict, check_input
 
 NoneType = type(None)
 
+
 class MinHashEncoder(BaseEstimator, TransformerMixin):
     """
     Encode string categorical features as a numeric array, minhash method
@@ -89,9 +90,7 @@ class MinHashEncoder(BaseEstimator, TransformerMixin):
         """
         return {"X_types": ["categorical"]}
 
-    def minhash(
-        self, string: str, n_components: int, ngram_range: Tuple[int, int]
-    ) -> np.array:
+    def _get_murmur_hash(self, string: str) -> np.array:
         """
         Encode a string using murmur hashing function.
 
@@ -108,10 +107,10 @@ class MinHashEncoder(BaseEstimator, TransformerMixin):
 
         Returns
         -------
-        array, shape (n_components, )
+        ndarray of shape (n_components, )
             The encoded string.
         """
-        min_hashes = np.ones(n_components) * np.infty
+        min_hashes = np.ones(self.n_components) * np.infty
         grams = get_unique_ngrams(string, self.ngram_range)
         if len(grams) == 0:
             grams = get_unique_ngrams(" Na ", self.ngram_range)
@@ -119,13 +118,13 @@ class MinHashEncoder(BaseEstimator, TransformerMixin):
             hash_array = np.array(
                 [
                     murmurhash3_32("".join(gram), seed=d, positive=True)
-                    for d in range(n_components)
+                    for d in range(self.n_components)
                 ]
             )
             min_hashes = np.minimum(min_hashes, hash_array)
         return min_hashes / (2**32 - 1)
 
-    def get_fast_hash(self, string: str) -> np.array:
+    def _get_fast_hash(self, string: str) -> np.array:
         """
         Encode a string with fast hashing function.
         fast hashing supports both min_hash and minmax_hash encoding.
@@ -137,7 +136,7 @@ class MinHashEncoder(BaseEstimator, TransformerMixin):
 
         Returns
         -------
-        np.array of shape (n_components, )
+        ndarray of shape (n_components, )
             The encoded string, using specified encoding scheme.
         """
         if self.minmax_hash:
@@ -172,6 +171,16 @@ class MinHashEncoder(BaseEstimator, TransformerMixin):
         MinHashEncoder
             The fitted MinHashEncoder instance.
         """
+        if self.hashing not in ["fast", "murmur"]:
+            raise ValueError(
+                f"Got hashing={self.hashing!r}, "
+                'but expected any of {"fast", "murmur"}. '
+            )
+        if self.handle_missing not in ["error", "zero_impute"]:
+            raise ValueError(
+                f"Got handle_missing={self.handle_missing!r}, but expected "
+                'any of {"error", "zero_impute"}. '
+            )
         self.count = 0
         self.hash_dict_ = LRUDict(capacity=self._capacity)
         return self
@@ -187,7 +196,7 @@ class MinHashEncoder(BaseEstimator, TransformerMixin):
 
         Returns
         -------
-        array, shape (n_samples, n_components)
+        ndarray of shape (n_samples, n_components)
             Transformed input.
         """
         X = check_input(X)
@@ -209,53 +218,42 @@ class MinHashEncoder(BaseEstimator, TransformerMixin):
             for k in range(X.shape[1]):
                 X_in = X[:, k].reshape(-1)
                 for i, x in enumerate(X_in):
-                    if (isinstance(x, float)  # true if x is a missing value
-                        or x is None):
+                    if (
+                        isinstance(x, float)  # true if x is a missing value
+                        or x is None
+                    ):
+                        is_nan_idx = True
+                    elif len(x) == 0:
                         is_nan_idx = True
                     elif x not in self.hash_dict_:
                         X_out[i, k * self.n_components : counter] = self.hash_dict_[
                             x
-                        ] = self.get_fast_hash(x)
+                        ] = self._get_fast_hash(x)
                     else:
                         X_out[i, k * self.n_components : counter] = self.hash_dict_[x]
                 counter += self.n_components
-        elif self.hashing == "murmur":
+        if self.hashing == "murmur":
             X_out = np.zeros((len(X[:]), self.n_components * X.shape[1]))
             counter = self.n_components
             for k in range(X.shape[1]):
                 X_in = X[:, k].reshape(-1)
                 for i, x in enumerate(X_in):
-                    if isinstance(x, float):
+                    if isinstance(x, float) or x is None:
+                        is_nan_idx = True
+                    elif len(x) == 0:
                         is_nan_idx = True
                     elif x not in self.hash_dict_:
                         X_out[i, k * self.n_components : counter] = self.hash_dict_[
                             x
-                        ] = self.minhash(
-                            x,
-                            n_components=self.n_components,
-                            ngram_range=self.ngram_range,
-                        )
+                        ] = self._get_murmur_hash(x)
                     else:
                         X_out[i, k * self.n_components : counter] = self.hash_dict_[x]
                 counter += self.n_components
-        else:
-            raise ValueError(
-                f"Got hashing={self.hashing}, "
-                'but expected any of {"fast", "murmur"}. '
-            )
 
-        if self.handle_missing == "error":
-            if is_nan_idx:
-                raise ValueError(
-                    "Found missing values in input data; set "
-                    "handle_missing='zero_impute' "
-                    "to encode with missing values. "
-                )
-        elif self.handle_missing == "zero_impute":
-            pass
-        else:
+        if is_nan_idx and self.handle_missing == "error":
             raise ValueError(
-                f"Got handle_missing={self.handle_missing}, but expected "
-                'any of {"error", "zero_impute"}. '
+                "Found missing values in input data; set "
+                "handle_missing='zero_impute' "
+                "to encode with missing values. "
             )
         return X_out
diff --git a/dirty_cat/_string_distances.py b/dirty_cat/_string_distances.py
index 0822421..e17a5dd 100644
--- a/dirty_cat/_string_distances.py
+++ b/dirty_cat/_string_distances.py
@@ -66,8 +66,8 @@ def get_unique_ngrams(string: str, ngram_range: Tuple[int, int]):
     string : str
         The string to split in n-grams.
     ngram_range : tuple (min_n, max_n)
-    The lower and upper boundary of the range of n-values for different
-    n-grams to be extracted. All values of n such that min_n <= n <= max_n.
+        The lower and upper boundary of the range of n-values for different
+        n-grams to be extracted. All values of n such that min_n <= n <= max_n.
 
     Returns
     -------


Test patch:
diff --git a/dirty_cat/tests/test_docstrings.py b/dirty_cat/tests/test_docstrings.py
index 394e603..7177f69 100644
--- a/dirty_cat/tests/test_docstrings.py
+++ b/dirty_cat/tests/test_docstrings.py
@@ -28,8 +28,8 @@ DOCSTRING_TEMP_IGNORE_SET = {
     "dirty_cat._gap_encoder.GapEncoder.transform",
     "dirty_cat._minhash_encoder.MinHashEncoder",
     "dirty_cat._minhash_encoder.MinHashEncoder.fit",
-    "dirty_cat._minhash_encoder.MinHashEncoder.get_fast_hash",
-    "dirty_cat._minhash_encoder.MinHashEncoder.minhash",
+    "dirty_cat._minhash_encoder.MinHashEncoder._get_fast_hash",
+    "dirty_cat._minhash_encoder.MinHashEncoder._get_murmur_hash",
     "dirty_cat._minhash_encoder.MinHashEncoder.transform",
     "dirty_cat._similarity_encoder.SimilarityEncoder",
     "dirty_cat._similarity_encoder.SimilarityEncoder.fit",
diff --git a/dirty_cat/tests/test_minhash_encoder.py b/dirty_cat/tests/test_minhash_encoder.py
index c640fb0..a416046 100644
--- a/dirty_cat/tests/test_minhash_encoder.py
+++ b/dirty_cat/tests/test_minhash_encoder.py
@@ -1,46 +1,39 @@
 import random
-import time
 from string import ascii_lowercase
 
 import numpy as np
 import pandas as pd
 import pytest
-from sklearn.datasets import fetch_20newsgroups
 
 from dirty_cat import MinHashEncoder
-from dirty_cat.datasets import fetch_employee_salaries
 
 
-def test_MinHashEncoder(n_sample: int = 70) -> None:
-    X_txt = fetch_20newsgroups(subset="train")["data"]
-    X = np.array(X_txt[:n_sample])[:, None]
-
-    for minmax_hash in [True, False]:
-        for hashing in ["fast", "murmur"]:
-            if minmax_hash and hashing == "murmur":
-                pass  # not implemented
-
-            # Test output shape
-            encoder = MinHashEncoder(n_components=50, hashing=hashing)
-            encoder.fit(X)
-            y = encoder.transform(X)
-            assert y.shape == (n_sample, 50), str(y.shape)
-            assert len(set(y[0])) == 50
-
-            # Test same seed return the same output
-            encoder = MinHashEncoder(50, hashing=hashing)
-            encoder.fit(X)
-            y2 = encoder.transform(X)
-            np.testing.assert_array_equal(y, y2)
-
-            # Test min property
-            if not minmax_hash:
-                X_substring = [x[: x.find(" ")] for x in X[:, 0]]
-                X_substring = np.array(X_substring)[:, None]
-                encoder = MinHashEncoder(50, hashing=hashing)
-                encoder.fit(X_substring)
-                y_substring = encoder.transform(X_substring)
-                np.testing.assert_array_less(y - y_substring, 0.0001)
+@pytest.mark.parametrize(
+    "hashing, minmax_hash", [("fast", True), ("fast", False), ("murmur", False)]
+)
+def test_MinHashEncoder(hashing, minmax_hash) -> None:
+    X = np.array(["al ice", "b ob", "bob and alice", "alice and bob"])[:, None]
+    # Test output shape
+    encoder = MinHashEncoder(n_components=2, hashing=hashing)
+    encoder.fit(X)
+    y = encoder.transform(X)
+    assert y.shape == (4, 2), str(y.shape)
+    assert len(set(y[0])) == 2
+
+    # Test that using the same seed returns the same output
+    encoder2 = MinHashEncoder(2, hashing=hashing)
+    encoder2.fit(X)
+    y2 = encoder2.transform(X)
+    np.testing.assert_array_equal(y, y2)
+
+    # Test min property
+    if not minmax_hash:
+        X_substring = [x[: x.find(" ")] for x in X[:, 0]]
+        X_substring = np.array(X_substring)[:, None]
+        encoder3 = MinHashEncoder(2, hashing=hashing)
+        encoder3.fit(X_substring)
+        y_substring = encoder3.transform(X_substring)
+        np.testing.assert_array_less(y - y_substring, 0.001)
 
 
 def test_multiple_columns() -> None:
@@ -79,28 +72,42 @@ def test_input_type() -> None:
     enc.fit_transform(X)
 
 
-def profile_encoder(hashing: str = "fast", minmax_hash: bool = False) -> float:
-    # not a unit test
-    employee_salaries = fetch_employee_salaries()
-    df = employee_salaries.X
-    X = df[["employee_position_title"]]
-    t0 = time.time()
-    enc = MinHashEncoder(n_components=50, hashing=hashing, minmax_hash=minmax_hash)
+@pytest.mark.parametrize(
+    "hashing, minmax_hash", [("fast", True), ("fast", False), ("murmur", False)]
+)
+def test_encoder_params(hashing, minmax_hash) -> None:
+    MAX_LIMIT = 255  # extended ASCII Character set
+    i = 0
+    str_list = []
+    for i in range(100):
+        random_string = "aa"
+        for _ in range(100):
+            random_integer = random.randint(0, MAX_LIMIT)
+            random_string += chr(random_integer)
+            if random_integer < 50:
+                random_string += "  "
+        i += 1
+        str_list += [random_string]
+    X = np.array(str_list).reshape(100, 1)
+    enc = MinHashEncoder(
+        n_components=50, hashing=hashing, minmax_hash=minmax_hash, ngram_range=(3, 3)
+    )
     enc.fit(X)
     y = enc.transform(X)
     assert y.shape == (len(X), 50)
-    eta = time.time() - t0
-    return eta
+    X2 = np.array([["a", "", "c"]]).T
+    y2 = enc.transform(X2)
+    assert y2.shape == (len(X2), 50)
 
 
-@pytest.mark.parametrize(
-    "input_type, missing, hashing",
-    [
-        ["numpy", "error", "fast"],
-        ["pandas", "zero_impute", "murmur"],
-        ["numpy", "zero_impute", "fast"],
-    ],
-)
+input_types = ["numpy", "pandas"]
+missings = ["error", "zero_impute", "aaa"]
+hashings = ["fast", "murmur", "aaa"]
+
+
+@pytest.mark.parametrize("input_type", input_types)
+@pytest.mark.parametrize("missing", missings)
+@pytest.mark.parametrize("hashing", hashings)
 def test_missing_values(input_type: str, missing: str, hashing: str) -> None:
     X = ["Red", np.nan, "green", "blue", "green", "green", "blue", float("nan")]
     n = 3
@@ -114,33 +121,39 @@ def test_missing_values(input_type: str, missing: str, hashing: str) -> None:
     encoder = MinHashEncoder(
         n_components=n, hashing=hashing, minmax_hash=False, handle_missing=missing
     )
-    if missing == "error":
-        encoder.fit(X)
-        if input_type in ["numpy", "pandas"]:
-            with pytest.raises(ValueError, match=r"missing values in input"):
-                encoder.transform(X)
-    elif missing == "zero_impute":
-        encoder.fit(X)
-        y = encoder.transform(X)
-        if input_type == "list":
-            assert np.allclose(y[1], y[-1])
-        else:
+
+    if hashing == "aaa":
+        with pytest.raises(ValueError, match=r"Got hashing="):
+            encoder.fit_transform(X)
+    else:
+        if missing == "error":
+            if input_type in ["numpy", "pandas"]:
+                with pytest.raises(
+                    ValueError, match=r"Found missing values in input data; set"
+                ):
+                    encoder.fit_transform(X)
+        elif missing == "zero_impute":
+            y = encoder.fit_transform(X)
             assert np.array_equal(y[1], z)
             assert np.array_equal(y[-1], z)
-    else:
-        with pytest.raises(ValueError, match=r"expected any of"):
-            encoder.fit_transform(X)
+        else:
+            with pytest.raises(ValueError, match=r"Got handle_missing="):
+                encoder.fit_transform(X)
     return
 
 
 def test_missing_values_none():
     # Test that "None" is also understood as a missing value
-    a = np.array([['a', 'b', None, 'c']], dtype=object).T
+    a = np.array([["a", "b", None, "c"]], dtype=object).T
 
     enc = MinHashEncoder()
     d = enc.fit_transform(a)
     np.testing.assert_array_equal(d[2], 0)
 
+    e = np.array([["a", "b", "", "c"]], dtype=object).T
+    f = enc.fit_transform(e)
+    np.testing.assert_array_equal(f[2], 0)
+
 
 def test_cache_overflow() -> None:
     # Regression test for cache overflow resulting in -1s in encoding
@@ -156,17 +169,3 @@ def test_cache_overflow() -> None:
     y = encoder.fit_transform(raw_data)
 
     assert len(y[y == -1.0]) == 0
-
-
-@pytest.mark.parametrize("idx", range(3))
-def test_min_hash_encoder_hashing_fast_minmax_hash(idx):
-    print(f"{profile_encoder(hashing='fast', minmax_hash=True):.4} seconds")
-
-
-@pytest.mark.parametrize("idx", range(3))
-def test_min_hash_encoder_hashing_fast(idx):
-    print(f"{profile_encoder(hashing='fast'):.4} seconds")
-
-
-def test_min_hash_encoder_mumur():
-    print(f"{profile_encoder(hashing='murmur'):.4} seconds")