Skip to content

Conversation

@nworb-cire
Copy link

This updates the estimator to comply with scikit-learn’s SelectorMixin API. With this change, the estimator behaves consistently with scikit-learn’s native selectors, allowing it to plug seamlessly into any pipeline or utility that expects the standard selector interface.

Importantly, the estimator’s underlying logic and outputs remain unchanged. The SelectorMixin class has been part of scikit-learn since version 0.11 (released in 2012), so this update introduces no backward-compatibility concerns.

@solegalli
Copy link
Collaborator

Hi @nworb-cire

I am Sole, helping @danielhomola maintain the library.

The proposed change makes sense, thanks a lot for the PR.

Could you please rebase master onto this? We just migrated from unittest to pytest and added automatic testing to make reviewing easier.

It would also be helpful if you could add a few tests that evaluate that the addition you are making here result in the expected functionality.

I tried to look at the source code of SelectorMixin vs TransformerMixin earlier today, but the website was on maintenance, so I don't have on the top of my head the differences brought forward by this change, but these are the ones that we'd need tests for.

* origin/master:
  [MNT] add automatic test workflow (scikit-learn-contrib#146)
  add error message when user passes decision trees (scikit-learn-contrib#141)
  migrate from unittest to pytest (scikit-learn-contrib#140)
  Bump pypa/gh-action-pypi-publish in /.github/workflows (scikit-learn-contrib#144)
  evaluate features only after 5th iteration (scikit-learn-contrib#137)
  add gitignore (scikit-learn-contrib#139)
  fix typo in docstring (scikit-learn-contrib#138)
@nworb-cire
Copy link
Author

One remaining API gap I have noticed is that the BorutaPy.transform method now overrides SelectorMixin.transform to expose two parameters. Following sklearn conventions, it might be better to make weak an attribute (affecting _get_support_mask; this could be done in a backwards compatible way if desired), in which case we could remove the custom transform method and fall back to the SelectorMixin's pandas-aware implementation. I can do that as part of this PR or as a follow-up, whichever you'd prefer.

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.

2 participants