fixed bug in drop_features

[luismavs]

Hi,

I found a bug in the DropFeatures transformer, module drop_features.py. The check (number of columns) in the .fit() method may fail unintentionally if features_to_drop is a string.

from feature_engine.selection import DropFeatures
import pandas as pd

df = pd.DataFrame({'aa':[1,2], 'bb': [3,4]})
transformer = DropFeatures(features_to_drop="aa")
X = transformer.fit_transform(df)

To fix this, I´ve applied the conversion of features_to_drop using the _find_all_variables method.

[solegalli]

Hey @luismavs thank you so much!

This is very helpful.

Could I ask you a few small things?

First, we need to fix code style. Here the error:

feature_engine/selection/drop_features.py:8:89: E501 line too long (89 > 88 characters)
feature_engine/selection/drop_features.py:10:1: E302 expected 2 blank lines, found 1

In short, we need to shorten 1 line and add a blank line to the other.

And also, could you use the syntax that you have in your comment to create a new test?

So that we ensure that we do not incur accidentally in the same error if we modify the class at some point.

Thank you!!

[solegalli]

could you please remove the commented line?

[luismavs]

Hi,

I´ve pushed a new commit with the formatting changes you asked for, and an expanded version of the test test_drop_1_variable(...) which demonstrates the fix ( len('Marks') = len(X.columns) which was triggering the bug).

[solegalli]

Hey @luismavs

Thanks! There are still some codestyle errors.

Do you have installed a tool called black?

If you do, you just need to execute black feature-engine tests and it will resolve it automatically (almost always).

To reproduce the errors locally, you need to install flake and then execute flake8 feature-engine tests

Check the docs here

These are the errors if you want to resolve manually:

feature_engine/selection/drop_features.py:86:56: W291 trailing whitespace
feature_engine/selection/drop_features.py:87:53: E128 continuation line under-indented for visual indent
feature_engine/selection/drop_features.py:112:1: W391 blank line at end of file
tests/test_selection/test_drop_features.py:38:1: E303 too many blank lines (3)
tests/test_selection/test_drop_features.py:98:1: W391 blank line at end of file
stylechecks: exit 1 (3.46 seconds) /home/circleci/project> flake8 feature_engine tests pid=454
  stylechecks: FAIL code 1 (6.24=setup[2.78]+cmd[3.46] seconds)
  evaluation failed :( (6.33 seconds)


Exited with code exit status 1

CircleCI received exit code 1

[codecov]

Codecov Report

Merging #596 (9083fcf) into main (a0106c9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #596   +/-   ##
=======================================
  Coverage   97.79%   97.79%           
=======================================
  Files          90       90           
  Lines        3496     3497    +1     
  Branches      684      684           
=======================================
+ Hits         3419     3420    +1     
  Misses         29       29           
  Partials       48       48           

Impacted Files	Coverage Δ
feature_engine/selection/drop_features.py	100.00% <100.00%> (ø)

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

[luismavs]

Hey, code has now been properly formatted with black, looks like good to go.

[solegalli]

Hi @luismavs

This is looking really good. Thank you!

I've got a couple of cosmetic requests and a kind of important favour to ask for the test.

Would you be able to have a look?

Thank you!

We are almost there :)

[solegalli]

Hi, this looks good, but it took me a while to understand what this test was about, and why it is different from that in line 8.

Could we, instead of making it part of the current test, have a different test, just for this issue, with a clear name of what is being tested, and also in a comment add a link to the issue or PR?

[solegalli]

could we please add an assert to compare the result of the transformation with what we expect from it? see line 27

[solegalli]

this file was probably modified with black, but it is not really part of this PR. Could we try and remove it please?

[solegalli]

Thank you @luismavs !!

It's all done now.

That was a nasty bug you found :)