Applying to iloc of pandas data frame

[tetrar124]

Description

Bug fix for selecting columns and add new features for the name of the columns of pandas data frame.

Related issues or pull requests

Pull Request Checklist

• Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file (if applicable)
• Added appropriate unit test functions in the ./mlxtend/*/tests directories (if applicable)
• Modify documentation in the corresponding Jupyter Notebook under mlxtend/docs/sources/ (if applicable)
• Ran nosetests ./mlxtend -sv and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g., nosetests ./mlxtend/classifier/tests/test_stacking_cv_classifier.py -sv)
• Checked for style issues by running flake8 ./mlxtend

[pep8speaks]

Hello @tetrar124! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 13, 2019 at 07:57 Hours UTC

[rasbt]

Thanks for the PR. However, instead doing a try-except, I think the following could solve the problem better? (Haven't tested it, but what do you think? Does that cover your use case issue?)

        if hasattr(X, 'loc') or hasattr(X, 'iloc'):
            types = {type(i) for i in self.cols}
            if len(types) > 1:
                raise ValueError('Elements in `cols` should be all of the same data type.')
            if isinstance(cols[0], int):
                t = X.iloc[:, self.cols].values
            elif isinstance(cols[0], str):
                t = X.loc[:, self.cols].values
           else:
               raise ValueError('Elements in `cols` should be either `int` or `str`.')

[tetrar124]

I checked your code on my environment, your code works well (I changed only cols[0] to self.cols[0]).
And I added 2 lines in this code for using a tuple with iloc because of using a tuple in the user's guide.
http://rasbt.github.io/mlxtend/user_guide/feature_selection/ColumnSelector/

        if hasattr(X, 'loc') or hasattr(X, 'iloc'):
            if type(self.cols) == tuple:
               self.cols = list(self.cols)
            types = {type(i) for i in self.cols}
            if len(types) > 1:
                raise ValueError('Elements in `cols` should be all of the same data type.')
            if isinstance(self.cols[0], int):
                t = X.iloc[:, self.cols].values
            elif isinstance(self.cols[0], str):
                t = X.loc[:, self.cols].values
            else:
               raise ValueError('Elements in `cols` should be either `int` or `str`.')

Thank you for your cooperation.

[rasbt]

Thanks for working on this PR! Just as a tip to make the debugging smoother/faster (since the CI is very slow), you can also test your code locally by running

nosetests mlxtend/feature_selection

to see if there are any issues with the code.

[coveralls]

Coverage decreased (-0.009%) to 91.587% when pulling 09169ef on tetrar124:patch-4 into 6b57a73 on rasbt:master.

[tetrar124]

Sorry, I return elif to if. The reason for using elif is only for the coverage. I checked new lines with nose and flask8, and there are no errors other than 'line too long' from the new comment lines for ValueError.

[rasbt]

Ok thanks :)! Btw don't worry about minor changes regarding the coverage too much. Can you please make the line < 79 chars though (splitting it over two lines) -- we decided to strictly enforce PEP8 at some point so that the library won't become too messy over time by making exceptions.

[rasbt]

Could you also add an additional unit test for using a DataFrame with integer column names to make sure it works correctly? (I think we don't have that one yet) :)

[tetrar124]

I made the data frame with columns whose names were only integer values, and I checked the type of these columns names showed int.
In this situation, there was no bug but this code has worked only as the mode of iloc.
(I think it was correct work by this code)

[rasbt]

Thanks for testing. We should add a unit test though so that we can ensure that the behavior doesn't break in future.

I just added a unit test and changelog entry to this PR. Let me know if you have any further comments. Otherwise, I think it's good to merge if/when the CI tests pass. Thanks for the PR!