Skip to content

Implement Correlation with target as a selection method in SmartCorrelatedSelection - #827

Merged
solegalli merged 11 commits into
feature-engine:mainfrom
ClaudioSalvatoreArcidiacono:826-implement-corr-with-target-selection-method-smartcorrelatedselection
Mar 20, 2025
Merged

Implement Correlation with target as a selection method in SmartCorrelatedSelection#827
solegalli merged 11 commits into
feature-engine:mainfrom
ClaudioSalvatoreArcidiacono:826-implement-corr-with-target-selection-method-smartcorrelatedselection

Conversation

@ClaudioSalvatoreArcidiacono

Copy link
Copy Markdown
Contributor

Fixes #826

@codecov

codecov Bot commented Dec 2, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.03%. Comparing base (fd62305) to head (3bf2a1f).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #827   +/-   ##
=======================================
  Coverage   98.03%   98.03%           
=======================================
  Files         111      111           
  Lines        4623     4626    +3     
  Branches      729      730    +1     
=======================================
+ Hits         4532     4535    +3     
  Misses         56       56           
  Partials       35       35           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@solegalli

Copy link
Copy Markdown
Collaborator

Hi @ClaudioSalvatoreArcidiacono

Thanks for this PR. This is almost good to go. We need 2 extra bits:

  1. could you please rebase main so we take over the changes of sklearn tags and the new tests for the different sklearn versions?

  2. could you please expand the user guide by adding the extra discrimination method that you include here, and also an example?

This is the user guide file that needs updating: https://github.com/feature-engine/feature_engine/blob/main/docs/user_guide/selection/SmartCorrelatedSelection.rst

If would be great if you add the "theory" about the selection method at the top of the file and then an example following the example with the headline "Performance".

Thank you!

@ClaudioSalvatoreArcidiacono

Copy link
Copy Markdown
Contributor Author

Hey @solegalli, apologies for my late reply, I have been terribly busy lately.

Thanks a lot for reviewing my MR, I have implemented the changed you have requested, let me know if you would like to see more changes :).

Comment thread docs/user_guide/selection/SmartCorrelatedSelection.rst Outdated
Comment thread docs/user_guide/selection/SmartCorrelatedSelection.rst Outdated
.. code:: python

{'var_7': {'var_4', 'var_6', 'var_9'}, 'var_0': {'var_8'}}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this code, I think it would ne nice to show that the correlation of var_0 with the target is higher than the correlation of var_8 with the target. Would you mind adding that @ClaudioSalvatoreArcidiacono ?

Comment thread feature_engine/selection/smart_correlation_selection.py Outdated
@solegalli

Copy link
Copy Markdown
Collaborator

Thank you @ClaudioSalvatoreArcidiacono ! This is almost there. I propose a few minor edits and a paragraph needs updating. That's all. If you agree, give me a shout when it's done so I can merge :)

Comment thread docs/user_guide/selection/SmartCorrelatedSelection.rst Outdated
Co-authored-by: Soledad Galli <solegalli@protonmail.com>
Co-authored-by: Soledad Galli <solegalli@protonmail.com>
Co-authored-by: Soledad Galli <solegalli@protonmail.com>
Co-authored-by: Soledad Galli <solegalli@protonmail.com>
…rrelatedselection' of https://github.com/ClaudioSalvatoreArcidiacono/feature_engine into 826-implement-corr-with-target-selection-method-smartcorrelatedselection
@ClaudioSalvatoreArcidiacono

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing the MR @solegalli, all comments have been addressed.

@solegalli
solegalli merged commit 589c413 into feature-engine:main Mar 20, 2025
@solegalli

Copy link
Copy Markdown
Collaborator

Thank you @ClaudioSalvatoreArcidiacono !

This was an awesome contribution!

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.

Feature Request: Add Correlation with target as selection method for SmartCorrelatedSelection

2 participants