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

Add fit parameters passing #391

Merged
merged 22 commits into from
Jan 12, 2024

Conversation

sami-ka
Copy link
Contributor

@sami-ka sami-ka commented Dec 28, 2023

Description

In the MAPIE code, the fit function does not allow to pass parameters usable at fit time for base estimator. This is the case for early stopping in LightGBM for instance.

Fixes partially #212 (fit_params only)

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Add test with passing monitor parameter in fit function of GradientBoostingRegressor
  • Check that the other tests still work

Checklist

  • I have read the contributing guidelines
  • I have updated the HISTORY.rst and AUTHORS.rst files
  • Linting passes successfully : make lint
  • Typing passes successfully : make type-check
  • Unit tests pass successfully : make tests
  • Coverage is 100% : make coverage
  • Documentation builds successfully : make doc

@sami-ka sami-ka marked this pull request as ready for review December 29, 2023 10:17
@codecov-commenter
Copy link

codecov-commenter commented Jan 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (614293e) 100.00% compared to head (a4adcb6) 100.00%.
Report is 177 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##            master      #391    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           39        39            
  Lines         4616      4815   +199     
  Branches       487       800   +313     
==========================================
+ Hits          4616      4815   +199     

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

Copy link
Collaborator

@LacombeLouis LacombeLouis left a comment

Choose a reason for hiding this comment

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

Hi @sami-ka,
This is great! Thank you for contributing to MAPIE.
I see that you've added for all the instances for **fit_params.
One small issue is that you've currently only implemented one test. I suggest adding tests for classification and regression.
Thank you!

Copy link
Collaborator

@thibaultcordier thibaultcordier left a comment

Choose a reason for hiding this comment

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

Thank you very much for this proposal, which will extend the use of MAPIE to more estimators! Here is my feedback to complete this PR:

  • add unit tests for calibration, classification and regression (as you did for quantile regression). No need for time series.
  • some writing style suggestion.

mapie/regression/quantile_regression.py Show resolved Hide resolved
mapie/regression/regression.py Outdated Show resolved Hide resolved
mapie/classification.py Outdated Show resolved Hide resolved
mapie/tests/test_quantile_regression.py Outdated Show resolved Hide resolved
requirements.dev.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@thibaultcordier thibaultcordier left a comment

Choose a reason for hiding this comment

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

All right, then! We're about to wrap up this PR. I'd like to suggest a few changes to help us maintain your code in the future.

I see that we've instantiated the gb object at the start of the test_quantile_regression.py file, but I think it would be better to do this directly in your tests in the other files (as it won't be used elsewhere except for test_quantile_regression.py).

mapie/tests/test_calibration.py Outdated Show resolved Hide resolved
mapie/tests/test_calibration.py Outdated Show resolved Hide resolved
mapie/tests/test_classification.py Outdated Show resolved Hide resolved
mapie/tests/test_classification.py Outdated Show resolved Hide resolved
mapie/tests/test_calibration.py Show resolved Hide resolved
mapie/tests/test_regression.py Outdated Show resolved Hide resolved
mapie/tests/test_calibration.py Outdated Show resolved Hide resolved
mapie/tests/test_classification.py Outdated Show resolved Hide resolved
mapie/tests/test_quantile_regression.py Outdated Show resolved Hide resolved
mapie/tests/test_regression.py Outdated Show resolved Hide resolved
@thibaultcordier thibaultcordier added Enhancement Type: enhancement (new feature or request) Source: contributors Proposed by contributors. labels Jan 3, 2024
@sami-ka
Copy link
Contributor Author

sami-ka commented Jan 3, 2024

All right, then! We're about to wrap up this PR. I'd like to suggest a few changes to help us maintain your code in the future.

I see that we've instantiated the gb object at the start of the test_quantile_regression.py file, but I think it would be better to do this directly in your tests in the other files (as it won't be used elsewhere except for test_quantile_regression.py).

You are right, it would definitely be more relevant inside the tests. Thanks for the suggestion!

Copy link
Collaborator

@thibaultcordier thibaultcordier left a comment

Choose a reason for hiding this comment

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

Thank you very much @sami-ka for your qualitative contribution to MAPIE. As far as I'm concerned, everything's fine, even if I added a comment (thanks in advance if you can add this point). @vincentblot28 , @LacombeLouis if you want to see before the merge.

mapie/tests/test_quantile_regression.py Outdated Show resolved Hide resolved
Co-authored-by: Thibault Cordier <[email protected]>
@sami-ka
Copy link
Contributor Author

sami-ka commented Jan 12, 2024

@thibaultcordier Is there something else to do? Could this PR be merged?

@thibaultcordier
Copy link
Collaborator

@sami-ka I'll let @LacombeLouis confirm that he has no more change requests. He will then merge your PR.

@LacombeLouis LacombeLouis merged commit 9f52151 into scikit-learn-contrib:master Jan 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Type: enhancement (new feature or request) Source: contributors Proposed by contributors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants