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

Allow MapieRegressor to use the optimal estimation strategy for the bounds of the prediction intervals #387

Merged
merged 9 commits into from
Dec 20, 2023

Conversation

thibaultcordier
Copy link
Collaborator

@thibaultcordier thibaultcordier commented Dec 15, 2023

Description

In the MAPIE code, only the predict method of MapieTimeSeriesRegressor class can use optimize_beta to optimize the bounds of the prediction interval.

Propose the integration of the same feature into the MapieRegressor class (deleguate this feature in ConformityScore class). Merge the two predict methods to create a single method for easier maintenance.

Fixes #384
Fixes #389

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

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

  • Adapt test for optimize_beta in MapieTimeSeriesRegressor
  • 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

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            Coverage Diff             @@
##            master      #387    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           39        39            
  Lines         4616      4616            
  Branches       487       758   +271     
==========================================
  Hits          4616      4616            

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

@thibaultcordier thibaultcordier marked this pull request as ready for review December 15, 2023 14:09
@thibaultcordier
Copy link
Collaborator Author

Note: after this PR, we should open an issue to remove MapieRegressor warning when using this strategy and add an example of how to use this feature.

@thibaultcordier
Copy link
Collaborator Author

This PR proposes solution for issue #389.

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.

I think this is a good PR as we allow MAPIE users to explore different capabilities. I think it would be good to also add an issue so that we create a notebook or example to show the usage of beta_optimizer. This would be so that we show how this influences the interval bounds (and also for our own knowledge).

if method == "plus":
alpha_low = alpha_np if self.sym else alpha_np / 2
alpha_up = 1 - alpha_np if self.sym else 1 - alpha_np / 2
alpha_low = alpha_np if self.sym else beta_np
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it a little weird to call the bound beta_np. It really associates it with the beta optimizer. Could we not simply call it the bound?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a possibility. Up until now, we've always done it that way. This could be the subject of an issue if the internal naming system poses problems in the long term.

@thibaultcordier thibaultcordier merged commit 960d982 into master Dec 20, 2023
8 checks passed
@Valentin-Laurent Valentin-Laurent deleted the 384-regressor-predict-with-optimize-beta branch November 12, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants