Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

recursive floating for SequentialFeatureSelector #262

Merged
merged 7 commits into from
Oct 31, 2017
Merged

Conversation

rasbt
Copy link
Owner

@rasbt rasbt commented Oct 12, 2017

Description

Adds an recursive_floating parameter to the SequentialFeatureSelector:

recursive_floating : bool (default=False)
    If `True`, uses the floating behavior described in
    Novovicova & Kittler (1994) for the `floating=True` variants.
    That is, the continuation of conditional exclusion in SFFS
    (skipping inclusion if feature removal leads to better performance).
    Similarly, the conditional inclusion is continued in SFBS
    (and consequently the the exclusion is skipped) as long as it leads
    to improvements in performance.
    Note: `recursive_floating` will be set `True` in future versions
    of mlxtend (mlxtend > 0.10).

Related issues or pull requests

Fixes #243

Pull Request requirements

  • Added appropriate unit test functions in the ./mlxtend/*/tests directories
  • Ran nosetests ./mlxtend -sv and make sure that all unit tests pass
  • Checked the test coverage by running nosetests ./mlxtend --with-coverage
  • Checked for style issues by running flake8 ./mlxtend
  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file
  • Modify documentation in the appropriate location under mlxtend/docs/sources/ (optional)
  • Checked that the Travis-CI build passed at https://travis-ci.org/rasbt/mlxtend

More todos:

  • add a deprecation warning to the default setting recursive_floating=False (only if floating=True to avoid being too annoying) mentioning that this will be recursive_floating=True in mlxtend >= v0.10

@rasbt rasbt changed the title recursive floating recursive floating for SequentialFeatureSelector Oct 12, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 90.895% when pulling 8f22f4b on recursive-floating into 5735e00 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 90.893% when pulling 8f22f4b on recursive-floating into 5735e00 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 90.893% when pulling bb9af54 on recursive-floating into 5735e00 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 90.893% when pulling 1550820 on recursive-floating into 5735e00 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 90.893% when pulling 4e3ca62 on recursive-floating into 5735e00 on master.

@rasbt
Copy link
Owner Author

rasbt commented Oct 14, 2017

I simplified the implementation a little bit using two nested while-loops now. This is doing exactly the same as before with recursive_floating=True based on my debugger, but is now hopefully a bit more intuitive to read. Note that I removed the recursive_floating parameter and enabled the implementation by default now as the implementation is already complicated enough :P (also I didn't notice any differences in the test scenarios; I believe it could improve the feature selection in certain edge cases though)

@rasbt
Copy link
Owner Author

rasbt commented Oct 14, 2017

just saw that we had the discussion in the other thread, so I am just pinging you here ;) @nd26

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.849% when pulling 021b930 on recursive-floating into 5735e00 on master.

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.

None yet

2 participants