Skip to content

MAINT rename base_estimator in _BaseChain subclasses#30152

Merged
glemaitre merged 58 commits intoscikit-learn:mainfrom
SuccessMoses:rename_base_estimator
Jan 2, 2025
Merged

MAINT rename base_estimator in _BaseChain subclasses#30152
glemaitre merged 58 commits intoscikit-learn:mainfrom
SuccessMoses:rename_base_estimator

Conversation

@SuccessMoses
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #29620 using the process followed in a similar pull request #23819

What does this implement/fix? Explain your changes.

This PR continues from the stalled PR #29682.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 25, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: b382bdd. Link to the linter CI: here

@SuccessMoses
Copy link
Copy Markdown
Contributor Author

@adrinjalali , please help me fix the issue with changelog. This is my first time making this kind of pull request. I added my changes to doc/whats_new/v1.5.rst but still having issues

@adrinjalali
Copy link
Copy Markdown
Member

We've changed our changelog system, here's the new info: https://github.com/scikit-learn/scikit-learn/blob/main/doc/whats_new/upcoming_changes/README.md

You also need to enable pre-commit hooks so that the linters run before you commit.

@SuccessMoses
Copy link
Copy Markdown
Contributor Author

@glemaitre, I would appreciate it if you could review my pull request

@glemaitre
Copy link
Copy Markdown
Member

Could you fix the CI.

@SuccessMoses
Copy link
Copy Markdown
Contributor Author

CI is failing because of test/test_multioutput.py. I tried to run pytest locally before pushing, but pytest won't work.

How can i build scikit-learn?

Capture.

@glemaitre
Copy link
Copy Markdown
Member

You should follow the instructions in the contributing guideline:

https://scikit-learn.org/dev/developers/contributing.html#how-to-contribute

@SuccessMoses
Copy link
Copy Markdown
Contributor Author

@glemaitre there was a bug in the new code. I fixed it. The code passes pytest locally but not in the Azure pipeline. I don't know how to fix this. I am using windows 10.

Capture

@adrinjalali
Copy link
Copy Markdown
Member

You need to run the tests with SKLEARN_WARNINGS_AS_ERRORS=1 to get the same error as the CI.

You should read the log of the CI to see where the issue comes from. The stack trace of the CI shows you where the issue is.

Maybe @StefanieSenger could pair with you to help you move forward.

Comment thread sklearn/multioutput.py Outdated
@StefanieSenger
Copy link
Copy Markdown
Member

Hi @SuccessMoses,
thanks for working on this! :)

In order to make the CI tests pass, you need to fix the failing tests (use estimator instead of base_estimator). If you run pytest with the SKLEARN_WARNINGS_AS_ERRORS=1 flag, you should see which tests are failing locally.

Let me know if that doesn't work or you run into problems.

Copy link
Copy Markdown
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM I think.

Comment thread sklearn/multioutput.py Outdated
Comment thread sklearn/multioutput.py Outdated
SuccessMoses and others added 3 commits December 20, 2024 21:27
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Minor nits to update the versions, otherwise LGTM

Comment thread sklearn/multioutput.py Outdated
Comment thread sklearn/multioutput.py Outdated
Comment thread sklearn/multioutput.py Outdated
Comment thread sklearn/multioutput.py Outdated
Comment thread sklearn/tests/test_multioutput.py Outdated
SuccessMoses and others added 5 commits January 1, 2025 14:55
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@SuccessMoses
Copy link
Copy Markdown
Contributor Author

@thomasjpfan thanks for the review. I fixed the version number

@adrinjalali
Copy link
Copy Markdown
Member

You've got merge conflicts here @SuccessMoses

@glemaitre
Copy link
Copy Markdown
Member

Just pushing to fix the test. It was one remaining self.base_estimator due to the merge conflict. Let me review.

Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM if the CI get through this time.

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.

base_estimator in Chain classes while estimator is the convention in Bagging and MultiOutput classes?

6 participants