Skip to content

[MRG] Fixing test error coming from sklearn 1.8 release#358

Merged
tgnassou merged 14 commits intoscikit-adaptation:mainfrom
tgnassou:fix-test
Jan 15, 2026
Merged

[MRG] Fixing test error coming from sklearn 1.8 release#358
tgnassou merged 14 commits intoscikit-adaptation:mainfrom
tgnassou:fix-test

Conversation

@tgnassou
Copy link
Collaborator

No description provided.

@github-actions github-actions bot added the deep label Dec 19, 2025
skada/metrics.py Outdated
kwargs=None,
):
super().__init__()
super().__init__(None, 1 if greater_is_better else -1, kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

For readibility, I would pass the arguments, by name to know to which parameter we are affecting those values.

@tgnassou tgnassou changed the title [WIP] Fixing test error coming from sklearn 1.8 release [MRG] Fixing test error coming from sklearn 1.8 release Jan 6, 2026
@tgnassou
Copy link
Collaborator Author

tgnassou commented Jan 6, 2026

Using _BaseScorer change different things that I fixed, like the use of score_func properly.
I stop overriding the __call__ function but I'm wondering how to deal with the method_caller. Not sure if the way I did it is the good one. I just avoid using it, but maybe I should use it when we need to predict something with the estimator. Let me know wdyt @glemaitre !

@tgnassou tgnassou changed the title [MRG] Fixing test error coming from sklearn 1.8 release [WIP] Fixing test error coming from sklearn 1.8 release Jan 6, 2026
@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.93%. Comparing base (c6bb43e) to head (c9faaec).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #358      +/-   ##
==========================================
+ Coverage   95.91%   95.93%   +0.02%     
==========================================
  Files          66       66              
  Lines        7223     7236      +13     
==========================================
+ Hits         6928     6942      +14     
+ Misses        295      294       -1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tgnassou tgnassou changed the title [WIP] Fixing test error coming from sklearn 1.8 release [MRG] Fixing test error coming from sklearn 1.8 release Jan 7, 2026
Copy link
Collaborator

@rflamary rflamary left a comment

Choose a reason for hiding this comment

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

That's a very nice update/cleanup. I just have a few small comments

):
if base_estimator is None:
base_estimator = LogisticRegression(multi_class="multinomial")
base_estimator = LogisticRegression()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not needed anymore you are sure? This was used because the output for the prediction of teh scores is needed, please check that it does not break teh computatio,n of teh cose matrix (that it retsiurnes teh same a with mulyinomial)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With this code the multinomial loss will be used when n>=3. But the code is not breaking since LogisticRegression has a function predict_log_proba.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wdyt @rflamary ? right now the computation doesn't break.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok for me

torch = self.torch
if self.torch:
import torch
# torch = self.torch
Copy link
Collaborator

Choose a reason for hiding this comment

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

do ot keep this as comment

skada/metrics.py Outdated
from .utils import check_X_y_domain, extract_source_indices, source_target_split


# xxx(okachaiev): maybe it would be easier to reuse _BaseScorer?
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure easier but we need to do it now ;) @tgnassou you can cleanup teh comments maybe?

@tgnassou tgnassou merged commit 966861a into scikit-adaptation:main Jan 15, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants