scikit-learn-contrib / MAPIE

A scikit-learn-compatible module to estimate prediction intervals and control risks based on conformal predictions.
https://mapie.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
1.2k stars 99 forks source link

Add fit parameters passing #391

Closed sami-ka closed 5 months ago

sami-ka commented 6 months ago

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

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

Checklist

codecov-commenter commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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 ```diff @@ Coverage Diff @@ ## master #391 +/- ## ========================================== Coverage 100.00% 100.00% ========================================== Files 39 39 Lines 4616 4815 +199 Branches 487 800 +313 ========================================== + Hits 4616 4815 +199 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sami-ka commented 6 months ago

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!

sami-ka commented 5 months ago

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

thibaultcordier commented 5 months ago

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