Skip to content

r_regression and abs_r_regression added #8353

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 10 commits into from
Closed

r_regression and abs_r_regression added #8353

wants to merge 10 commits into from

Conversation

DSLituiev
Copy link
Contributor

@DSLituiev DSLituiev commented Feb 13, 2017 •
edited
Loading

Reference Issue

Fixes #7195

What does this implement/fix? Explain your changes.

refactored as described in the issue:

  • the f_regression is split into two functions : f_regression per se and r_regression which precomputes Pearson R
  • abs_r_regression wrapper around r_regression added for use with SelectKBest

@jnothman
Copy link
Member

Could you please motivate this a bit more for the ignorant among us? Is abs_r_regression commonly used?

Your implementation is currently failing tests on all platforms.

You will need to add tests for your changes.

@DSLituiev
Copy link
Contributor Author

I beg pardon if i am making feel uncomfortable anyone. The rationale is to use abs_r_regression as a faster regressor compared to f_regression. It does not do any multiplication / power and no p-value calculation. It is just enough for feature selection using SelectKBest or based on a pre-defined p-value threshold as described in the issue #7195 and references therein.

@codecov
Copy link

codecov bot commented Feb 15, 2017 •
edited
Loading

Codecov Report

Merging #8353 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8353      +/-   ##
==========================================
+ Coverage   94.75%   94.75%   +<.01%     
==========================================
  Files         342      342              
  Lines       60801    60832      +31     
==========================================
+ Hits        57609    57640      +31     
  Misses       3192     3192
Impacted Files Coverage Δ
sklearn/feature_selection/univariate_selection.py 99.48% <100%> (+0.01%) ✅
...arn/feature_selection/tests/test_feature_select.py 99.75% <100%> (+0.01%) ✅
sklearn/feature_selection/init.py 100% <100%> (ø) ✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05b5b37...fa5dfe3. Read the comment docs.

@DSLituiev
Copy link
Contributor Author

@jnothman: any chance to merge this branch?

@jnothman
Copy link
Member

jnothman commented Mar 28, 2017 via email

@jnothman
Copy link
Member

jnothman commented Feb 1, 2018

Could you give some sense of the performance gains due to using r_regression vs f_regression? Perform a benchmark...

@amueller amueller added Needs Benchmarks A tag for the issues and PRs which require some benchmarks Stalled labels Aug 5, 2019
@jjerphan jjerphan mentioned this pull request May 9, 2020
Merged
@jjerphan
Copy link
Member

jjerphan commented May 9, 2020

I am currently trying a follow-up of this PR on #17169.

@cmarmo cmarmo added the Superseded PR has been replace by a newer PR label Nov 30, 2020
Base automatically changed from master to main January 22, 2021 10:49
@cmarmo
Copy link
Contributor

cmarmo commented Apr 21, 2021

Closed by #17169.

@cmarmo cmarmo closed this Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
module:feature_selection Needs Benchmarks A tag for the issues and PRs which require some benchmarks Stalled Superseded PR has been replace by a newer PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[refactor] f_regression into r_regression and a wrapper for performance
5 participants
@DSLituiev @jnothman @jjerphan @cmarmo @amueller