[ENH] Add keywords parameter to StringSimilarityEncoder by glevv · Pull Request #550 · feature-engine/feature_engine

glevv · 2022-10-28T15:37:43Z

Added keywords support for StringSimilarityEncoder.

Keywords allow user to introduce some domain knowledge (if any) into encoder

codecov · 2022-10-28T16:29:16Z

Codecov Report

Merging #550 (7c230f3) into main (22e6fbf) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #550   +/-   ##
=======================================
  Coverage   97.47%   97.47%           
=======================================
  Files          90       90           
  Lines        3480     3492   +12     
  Branches      681      687    +6     
=======================================
+ Hits         3392     3404   +12     
  Misses         32       32           
  Partials       56       56

Impacted Files	Coverage Δ
feature_engine/encoding/similarity_encoder.py	`98.82% <100.00%> (+0.19%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

glevv · 2022-10-29T18:39:02Z

I added a bunch of tests, seems good, but maybe it needs more

solegalli

Hi @glevv

Thank you for the enhancement. It looks good to me. I have mostly some minor comments.

And one question regarding how Nan in the train set should be handled. My understanding is:

"impute", replaces nan by "". So when creating the dummies, would "" be one of them?
"raise"; raises an error
"ignore": nan remain nan. But the current logic, won't make "" part of the dummies?

Thanks a lot for the contribution!

solegalli · 2022-10-31T09:28:56Z

+        to create similarity variables. Useful when someone has domain knowledge of the
+        problem. Could be defined only for several features; in this case for specified
+        features keywords will be used and most common categories will be used for
+        unspecified.


I don't understand the last sentence of this description. Would you be able to re-phrase it?

solegalli · 2022-10-31T09:33:35Z

-                )
-        elif self.missing_values == "impute":
-            for var in self.variables_:
+        for var in self.variables_:


are we removing the functionality to handle missing data? I guess, not because the tests pass.

But regarding this code:

feature_engine/feature_engine/encoding/similarity_encoder.py

Line 236 in a2c315d

.replace("nan", "")

Are we not making nan part of the categories to appear as dummies?

solegalli · 2022-10-31T09:34:43Z

    assert tr.get_feature_names_out(input_features=input_features) == out
+
+
+def test_keywords_bad_type():


could we test more than just a string by using parametrize? maybe tuples, integers?

solegalli · 2022-10-31T09:35:32Z

+        StringSimilarityEncoder(keywords="hola")
+
+
+def test_keywords_bad_items():


same here, could we use parametrize and test more options?

solegalli · 2022-10-31T09:37:05Z

+        StringSimilarityEncoder(keywords={"column": "hola"})
+
+
+def test_keywords_dont_match(df_enc_big):


Could we either add columns that are in the data to the dictionary, or even better, test a second dictionary with a combination of columns that are in the data, and a column that is not?

Also, here we are testing that the variables/columns don't match, correct? can we adjust the name of the test?

solegalli · 2022-10-31T09:40:18Z

+    assert "var_B_F" not in X.columns
+
+
+def test_encode_full_keywords():


what do you mean by "full_keywords"?

solegalli · 2022-11-01T10:24:44Z

Hi @glevv

This is looking really good. Thank you so much for the quick turnaround.

I've got a couple of questions about the logic in fit:

fit logic:

self.encoder_dict_ is a dictionary of variables as keys and a list of dummies (or keywords in this case) as values.
self.keywords is the same thing, if I understand correctly: variables as keys and list of keywords as dummies.

So if the user defined all categorical variables, or the variables in the parameter variables in the self.keywords dict, in essence, self.encoder_dict_ and self.keyword dict are the same. In which case, we could just copy one to the other? instead of looping?
Would that not be faster?

At the moment, we've got the keywords dictionary after all the logic in fit():

feature_engine/feature_engine/encoding/similarity_encoder.py

Lines 256 to 258 in 05ae18c

    
           if self.keywords: 
        
               for var in self.keywords.keys(): 
        
                   self.encoder_dict_[var] = self.keywords[var]

This means that for the variables in the dictionary, we are fitting twice.

I wonder if there is a way in which we could avoid running the logic twice for those variables that are in the keywords dictionary?

Something like, if self.keyword exists and all the keys are in variables_ then just copy, alternatively, define the variables not in self.keywords.

imputation logic

Could we add a few tests to corroborate that:

when handle_missing="impute" and the fit() data has nan,, then "" is part of the dummies? is this the expected behaviour?
when handle_missing="ignore" and the fit() data has nan,, then no dummy is generated at the back of it?

In short, we need a test to differentiate the behaviour between handle_missing is impute or ignore, and the train set has nan values.

Finally, we've got another PR coming up that modifies the strings of this class: #547

Thanks a lot for your time!

glevv · 2022-11-01T11:34:28Z

Done

solegalli

Hi @glevv

Thank you for the code changes! I think this is almost there.

Would you mind rebasing to incorporate #547 and also have a look at my comment below?

Thanks a lot!

solegalli · 2022-11-07T10:21:02Z

                    .head(self.top_categories)
                    .index.tolist()
                )
+        if self.keywords:


If we update the dictionary here, and the dictionary keys contain all the variables in variables_, we are running all of the logic between 226 and this line, for no reason. Because those keywords will not be used.

Would it not be better to check, at around line 223 that:

if, all of variables_ are in keywords.keys(), then just copy dict.

If some of variables_ are in keywords.keys(), then apply the logic to find keywords in the remaining variables

If keywords is None, then apply the logic in all variables_

What do you think about this?

No, it will be worse. It will make logic more complicated (which in turn will mess up coverage and tests) and won't give any speed up.

This way it is straightforward and readable.

But I will look into it.

glevv · 2022-11-08T16:46:25Z

I implemented this logic, but tests are failing because some other transformers are not using Optional typing

see #555

solegalli · 2022-11-09T15:44:40Z

Sorry for the delay, I just merged #555

Logic changes look good to me!

solegalli · 2022-11-10T09:00:39Z

Hi @glevv

FYI: glevv#4

and we are then good to go.

Thanks for the contribution!!!

reword docstrings and errors

glevv added 14 commits October 28, 2022 15:25

keywords support for sim encodere

5534ac1

smallfix

8425b3a

added tests for keywords

56011c6

hotfix

431678d

small fix

33de650

revert

618bda0

Update test_similarity_encoder.py

42ab3a2

Update similarity_encoder.py

d5f3bf9

Update similarity_encoder.py

b9167d0

Update test_similarity_encoder.py

917ea3c

Update similarity_encoder.py

0858096

Update test_similarity_encoder.py

782a087

Update similarity_encoder.py

b2c371f

Update similarity_encoder.py

7eda2de

glevv changed the title ~~Sim enc kwds~~ [WIP] Add keywords parameter to StringSimilarityEncoder Oct 28, 2022

Update similarity_encoder.py

f69775a

glevv added 3 commits October 29, 2022 08:34

typing changes

ec01da3

Update test_similarity_encoder.py

2f89701

flake

a2c315d

glevv marked this pull request as ready for review October 29, 2022 18:39

glevv changed the title ~~[WIP] Add keywords parameter to StringSimilarityEncoder~~ [ENH] Add keywords parameter to StringSimilarityEncoder Oct 29, 2022

solegalli reviewed Oct 31, 2022

View reviewed changes

glevv added 6 commits October 31, 2022 17:54

revert to old logic

858e1c5

add parametrize

bedbf6a

flake fix

566d685

fix ignore case

bbba622

change of logic

87082d6

none check

472efdf

test update

05ae18c

glevv added 4 commits November 1, 2022 10:44

simplify dict update

aa31374

added tests for impute and ignore

f23069f

ignore case fix

7a30c6c

Update similarity_encoder.py

69277dd

solegalli reviewed Nov 7, 2022

View reviewed changes

glevv added 3 commits November 8, 2022 16:18

update logic

ee21c52

hotfix

a065d43

Merge branch 'feature-engine:main' into sim-enc-kwds

ddfb763

glevv and others added 4 commits November 9, 2022 19:34

Merge branch 'feature-engine:main' into sim-enc-kwds

dbaacf5

rewords keywords docstring

278700b

rewords error msgs

30b231b

rewords error message

f948dc1

Merge pull request #4 from feature-engine/sim-enc-kwds

7c230f3

reword docstrings and errors

solegalli merged commit 9a6fd8d into feature-engine:main Nov 10, 2022

glevv deleted the sim-enc-kwds branch November 10, 2022 11:33

		assert tr.get_feature_names_out(input_features=input_features) == out


		def test_keywords_bad_type():

		StringSimilarityEncoder(keywords="hola")


		def test_keywords_bad_items():

		StringSimilarityEncoder(keywords={"column": "hola"})


		def test_keywords_dont_match(df_enc_big):

		assert "var_B_F" not in X.columns


		def test_encode_full_keywords():

Uh oh!

Conversation

glevv commented Oct 28, 2022

Uh oh!

codecov Bot commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

glevv commented Oct 29, 2022

Uh oh!

solegalli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solegalli commented Nov 1, 2022

Uh oh!

glevv commented Nov 1, 2022

Uh oh!

solegalli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glevv Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glevv commented Nov 8, 2022

Uh oh!

solegalli commented Nov 9, 2022

Uh oh!

solegalli commented Nov 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Oct 28, 2022 •

edited

Loading

glevv Nov 7, 2022 •

edited

Loading