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

Feature range parameter support #1044

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

namanmistry
Copy link
Contributor

Description

As described in the issue Replace min_features and max_features in ExhaustiveFeatureSelector by feature_range and recipe #260 I have added support for new parameter feature_range in class ExhaustiveFeatureSelector. The priority of the parameters is still the same so it does not break the code base.

Related issues or pull requests

Fixes ``Replace min_features and max_features in ExhaustiveFeatureSelector by feature_range and recipe#260 partially. stillrecipe` parameter is remaining.

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)
  • Ran PYTHONPATH='.' pytest ./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., PYTHONPATH='.' pytest ./mlxtend/classifier/tests/test_stacking_cv_classifier.py -sv)
  • Checked for style issues by running flake8 ./mlxtend

@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Patch coverage: 20.00% and project coverage change: -0.08 ⚠️

Comparison is base (8ae4570) 77.27% compared to head (7d1a00c) 77.19%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1044      +/-   ##
==========================================
- Coverage   77.27%   77.19%   -0.08%     
==========================================
  Files         200      200              
  Lines       11308    11321      +13     
  Branches     1484     1487       +3     
==========================================
+ Hits         8738     8739       +1     
- Misses       2351     2362      +11     
- Partials      219      220       +1     
Impacted Files Coverage Δ
...d/feature_selection/exhaustive_feature_selector.py 70.58% <20.00%> (-4.70%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rasbt
Copy link
Owner

rasbt commented May 19, 2023

Thanks a lot for the PR! If you have time, could you add a few unit tests to make sure it works as intended?

@namanmistry
Copy link
Contributor Author

Hello @rasbt, Actually I am very new to open source and this is the first repo that I am contributing to. So could you please elaborate on how to add unit tests and possibly share some resources? Thank you.

@NimaSarajpoor
Copy link
Contributor

Hello @rasbt, Actually I am very new to open source and this is the first repo that I am contributing to. So could you please elaborate on how to add unit tests and possibly share some resources? Thank you.

I was just wandering and noticed your comment. You should add a test function in https://github.com/rasbt/mlxtend/blob/master/mlxtend/feature_selection/tests/test_exhaustive_feature_selector.py
to test whether the new changes work properly or not. I suggest that you go and checkout some of the existing test functions in the provided link to get some idea. Then, you should start write a test function. It is not always straightforward and you need to think how to assert the outcome of the function when user provides input considering the changes you applied.

I think @rasbt can definitely explain better when he gets a chance to get back to you. In the meantime, you may just go through the test functions in the provided link to better understand how tests are written.

@d-kleine
Copy link
Contributor

d-kleine commented Nov 1, 2024

@namanmistry Do you need further help with this?

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.

4 participants