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.

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

Feature mrmr #622

Merged
merged 41 commits into from
Mar 9, 2024
Merged

Feature mrmr #622

merged 41 commits into from
Mar 9, 2024

Conversation

fabioscantamburlo
Copy link
Contributor

@fabioscantamburlo fabioscantamburlo commented Feb 20, 2024 •
edited
Loading

Description

Implement MRMR feature selection model following the article:
MrMr By S. Mazzanti

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Remaining:

  • User guide.
  • Minor fixes.

Checklist:

  • My code follows the style guidelines (flake8)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (also to the readme.md)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added tests to check whether the new feature adheres to the sklearn convention
  • New and existing unit tests pass locally with my changes

If you feel your PR is ready for a review, ping @koaning or @MBrouns, @FBruzzesi

@fabioscantamburlo fabioscantamburlo marked this pull request as draft February 20, 2024 13:19
FBruzzesi
FBruzzesi reviewed Feb 20, 2024
View reviewed changes
Copy link
Collaborator

@FBruzzesi FBruzzesi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I left a few comments on style, formatting, phrasing (some nitpicks) and so on..

@koaning as I was somewhat involved in the code and logic implementation, I may be biased in reviewing it 😁

sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Show resolved Hide resolved
sklego/feature_selection/mrmr.py Show resolved Hide resolved
docs/_scripts/feature-selection.py Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Show resolved Hide resolved
sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
koaning
koaning reviewed Feb 20, 2024
View reviewed changes
Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

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

Left some quick notes, the main thing seems to be a missing documentation file.

fabioscantamburlo and others added 2 commits February 22, 2024 17:13
@fabioscantamburlo @FBruzzesi
Update sklego/feature_selection/mrmr.py
5d55a2e 
Fbruzzesi suggestion

Co-authored-by: Francesco Bruzzesi <42817048+FBruzzesi@users.noreply.github.com>
@fabioscantamburlo
Resolving comments on PR
2e107ef
@fabioscantamburlo fabioscantamburlo marked this pull request as ready for review March 1, 2024 15:54
FBruzzesi
FBruzzesi reviewed Mar 2, 2024
View reviewed changes
Copy link
Collaborator

@FBruzzesi FBruzzesi left a comment

Choose a reason for hiding this comment

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

A lot of tiny details

mkdocs.yaml Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
docs/user-guide/feature-selection.md Outdated Show resolved Hide resolved
docs/user-guide/feature-selection.md Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
sklego/feature_selection/mrmr.py Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@fabioscantamburlo
Copy link
Contributor Author

Gonna fix them asap 🙏

fabioscantamburlo and others added 8 commits March 3, 2024 10:01
@fabioscantamburlo @FBruzzesi
Update sklego/feature_selection/mrmr.py
249d17f 
Co-authored-by: Francesco Bruzzesi <42817048+FBruzzesi@users.noreply.github.com>
@fabioscantamburlo @FBruzzesi
Update sklego/feature_selection/mrmr.py
6c772d8 
Co-authored-by: Francesco Bruzzesi <42817048+FBruzzesi@users.noreply.github.com>
@fabioscantamburlo @FBruzzesi
Update docs/user-guide/feature-selection.md
b9b5bfc 
Co-authored-by: Francesco Bruzzesi <42817048+FBruzzesi@users.noreply.github.com>
@fabioscantamburlo @FBruzzesi
Update sklego/feature_selection/mrmr.py
86e0dc6 
Co-authored-by: Francesco Bruzzesi <42817048+FBruzzesi@users.noreply.github.com>
@fabioscantamburlo
resolve comments
f788026
@fabioscantamburlo
clean
87da50c
@fabioscantamburlo
suggestions + general rephrase
287977e
@fabioscantamburlo
Typo
8b4c40a
@fabioscantamburlo
Copy link
Contributor Author

fabioscantamburlo commented Mar 7, 2024 •
edited
Loading

I think PR is ready for review 😃
@koaning @FBruzzesi 🙏

koaning
koaning reviewed Mar 7, 2024
View reviewed changes
Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

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

I just have one nitpick. @FBruzzesi unless you have a strong opinion I'd say we merge this one in.

fabioscantamburlo and others added 2 commits March 9, 2024 13:57
@fabioscantamburlo @koaning
Update docs/user-guide/feature-selection.md
a1df5fe 
Co-authored-by: vincent d warmerdam  <vincentwarmerdam@gmail.com>
@fabioscantamburlo
Merge branch 'main' into FEATURE-MRMR
3b2bc7a
koaning
koaning approved these changes Mar 9, 2024
View reviewed changes
Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

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

I'll merge it for now. Release will have to wait for the hierarchical stuff, but yeah ... this'll be a nice addition for the v0.8.0 release 😄

@koaning koaning merged commit 64485a9 into koaning:main Mar 9, 2024
14 checks passed
@fabioscantamburlo
Copy link
Contributor Author

Great! Thanks for reviewing it 🙂

@FBruzzesi
Copy link
Collaborator

FBruzzesi commented Mar 9, 2024 •
edited
Loading

this'll be a nice addition for the v0.8.0 release 😄

@koaning do you think we should consider start adding tags for when a new feature is introduced? I would be in favor as we are rolling out features with a faster pace than the last few years 😁

@FBruzzesi FBruzzesi mentioned this pull request Mar 11, 2024
Closed
@fabioscantamburlo fabioscantamburlo deleted the FEATURE-MRMR branch June 13, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FBruzzesi FBruzzesi FBruzzesi left review comments

@koaning koaning koaning approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fabioscantamburlo @FBruzzesi @koaning