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

Most recent scikit-learn results in several failed unit tests #1091

Merged
merged 16 commits into from
Mar 30, 2024

Conversation

it176131
Copy link
Contributor

@it176131 it176131 commented Mar 30, 2024

Description

This handles the failing unit tests when scikit-learn is 1.3.1 or higher.

Related issues or pull requests

#1090
#1085
#1087

Pull Request Checklist

  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file (if applicable)
  • Added appropriate unit test functions in the ./mlxtend/*/tests directories (if applicable)
  • Modify documentation in the corresponding Jupyter Notebook under mlxtend/docs/sources/ (if applicable)
  • Ran PYTHONPATH='.' pytest ./mlxtend -sv and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g., PYTHONPATH='.' pytest ./mlxtend/classifier/tests/test_stacking_cv_classifier.py -sv)
  • Checked for style issues by running flake8 ./mlxtend

	- Marked :test:`test_StackingClassifier` as skip because scikit-learn implemented its own :class:`StackingClassifier` in 0.22.
@it176131 it176131 changed the title WIP Most recent scikit-learn results in several failed unit tests [WIP] Most recent scikit-learn results in several failed unit tests Mar 30, 2024
	- Skipping all failing unit tests related to :class:`StackingClassifier` as they don't align with `scikit-learn`'s implementation.
@it176131
Copy link
Contributor Author

it176131 commented Mar 30, 2024

As noted in the past few commit messages, scikit-learn implemented a StackingClassifier and StackingRegressor in 0.22 with a different API. Because of this, some of the current unit tests fail as they don't expose the necessary attributes.

@rasbt, should the mlxtend versions of the StackingClassifier and StackingRegressor be replaced with the scikit-learn implementations? Or renamed (so to not cause confusion) and modified to pass the tests?

	- Skipping failed unit tests because scikit-learn's StackingClassifier has built-in cross-validation support.
	- Updated scikit-learn version to 1.3.1

modified:   environment.yml
	- Updated scikit-learn version to 1.3.1

modified:   requirements.txt
	- Updated scikit-learn version to 1.3.1
	- Updated failing unit test to compare output directly instead of converting to numpy arrays (which results in errors unless the dtype is set to object).
	- Skipping `test_gridsearch_replace_mix` as it uses `StackingCVRegressor` when `scikit-learn` has its own implementation as of 0.22.
@it176131 it176131 marked this pull request as ready for review March 30, 2024 04:06
Copy link

codecov bot commented Mar 30, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 78.29%. Comparing base (ad06b2d) to head (d61928a).
Report is 8 commits behind head on master.

Files Patch % Lines
mlxtend/classifier/stacking_cv_classification.py 50.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1091      +/-   ##
==========================================
+ Coverage   77.26%   78.29%   +1.02%     
==========================================
  Files         200      196       -4     
  Lines       11297    11140     -157     
  Branches     1513     1404     -109     
==========================================
- Hits         8729     8722       -7     
+ Misses       2350     2200     -150     
  Partials      218      218              

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

@rasbt
Copy link
Owner

rasbt commented Mar 30, 2024

@rasbt, should the mlxtend versions of the StackingClassifier and StackingRegressor be replaced with the scikit-learn implementations? Or renamed (so to not cause confusion) and modified to pass the tests?

I would prefer to keep them as the scikit-learn port is not 1:1 equivalent, and the stacking classifiers here had some advantages (and disadvantages). I can update the unit tests to add them back, no worries!

@rasbt rasbt changed the title [WIP] Most recent scikit-learn results in several failed unit tests Most recent scikit-learn results in several failed unit tests Mar 30, 2024
@it176131
Copy link
Contributor Author

@rasbt, should the mlxtend versions of the StackingClassifier and StackingRegressor be replaced with the scikit-learn implementations? Or renamed (so to not cause confusion) and modified to pass the tests?

I would prefer to keep them as the scikit-learn port is not 1:1 equivalent, and the stacking classifiers here had some advantages (and disadvantages). I can update the unit tests to add them back, no worries!

Good team work! Once this is merged I'll sync my other branch and hopefully all the tests will pass 🤞

@rasbt rasbt merged commit e82c9c5 into rasbt:master Mar 30, 2024
4 checks passed
@it176131 it176131 deleted the issue_1090 branch March 30, 2024 19:56
@rasbt rasbt mentioned this pull request Mar 31, 2024
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