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

Compatibility with scikit-learn 1.6rc1 #720

Closed
wants to merge 6 commits into from
Closed

Conversation

FBruzzesi
Copy link
Collaborator

Description

Diff is simpler than expected:

  • Fix order of mixin's inheritance
  • Added the new __sklearn_tags__ to all the classes that changes the default tags
  • Created a utility function in sklego.common that perform validation either via validate_data (scikit>=1.6) or check_X_y and check_array (scikit < 1.6).

There are some checks which are not fully compatible, and one that might be buggy in this RC: check_sample_weight_equivalence seems to fail on every of our estimators that support sample_weight - it might be worth reporting upstream, but I could not pin the issue

Closes #719

@FBruzzesi FBruzzesi requested a review from koaning November 20, 2024 13:50
@FBruzzesi
Copy link
Collaborator Author

So current tests are all passing, which is definitly a good sign that we did not break anything. I tested with scikit-learn1.6rc1 locally (python3.10, ubuntu) and successfully

@koaning
Copy link
Owner

koaning commented Dec 11, 2024

Whoa. So many changes.

I guess the best check is to merge it now and to see if something breaks in the future, but nothing jumped out of the ordinary.

@koaning
Copy link
Owner

koaning commented Dec 11, 2024

Ah, I did the merge and now stuff seems breaking again 😓

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.

Changing state to reflect some tests break.

@koaning
Copy link
Owner

koaning commented Dec 11, 2024

This is ... odd. The tests locally in codespaces run just fine but they break in CI.

@FBruzzesi
Copy link
Collaborator Author

I will try to take a better look this weekend.. Should we pin scikit-learn version until we fix this?

@koaning
Copy link
Owner

koaning commented Dec 12, 2024

Ah, 1.6 is no longer a release candidate I think.

Let's delay version pinning unless folks start to submit issues. It could be that we are worrying about details that aren't a huge worry for folks.

@FBruzzesi
Copy link
Collaborator Author

FBruzzesi commented Dec 12, 2024

Yes, 1.6 is now definitly released 🙃

@FBruzzesi FBruzzesi closed this Dec 16, 2024
@koaning koaning deleted the patch/scikit-rc1.6 branch December 16, 2024 10:02
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.

Transition to scikit-learn 1.6
2 participants