-
Notifications
You must be signed in to change notification settings - Fork 112
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 Covariate Shift Conformal Regressor #151
base: master
Are you sure you want to change the base?
Conversation
Hey @gmartinonQM, please let me know how to prioritize tasks and would really appreciate feedback. Thanks! :) |
Thanks for the PR @RudrakshTuwani , for sure. Maybe we will take several days to discuss it internally. Stay tuned ! |
Hi @RudrakshTuwani , thank you very much for the PR ! The implementation and the notebook look really great. I am sorry for the delay of our feedback, we have been quite busy during the last past weeks. Before giving you a more thorough feedback by next week, just a few general comments :
|
Hello @vtaquet , thank you for getting back to me! No worries about the delay! :) |
Couldn't push to @vtaquet 's branch, so pulled the changes to the fork and re-opened PR. Still a WIP! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your contribution is relevant and appreciated for the evolution of MAPIE for conformal prediction with covariate shift. Moreover, it provides the basis for future developments towards conditional conformal prediction. In this perspective, I give you a list of suggestions to improve your code.
- I suggest you implement a new
WeightedConformalScore
class (in a new fileweighted_conformity_scores.py
), which inherits fromConformalScore
with all your current changes. - Thus the
CovariateShiftConformityScore
class will inherit fromWeightedConformalScore
and will be the only class that benefits from your changes without impacting the others. - In
MapieRegressor
class, I suggest to adapt the if condition with a test likeisinstance(self.conformity_score_function_, WeightedConformityScore)
. Is this possible in your opinion? Or am I missing something?
I remain available if you have any questions and thank you in advance for your feedback.
@@ -30,6 +32,7 @@ def __init__( | |||
- get_estimation_distribution and | |||
- get_signed_conformity_scores | |||
by default True. | |||
compute_score_weights : TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to define what is compute_score_weights
|
||
@abstractmethod | ||
def get_signed_conformity_scores( | ||
self, y: ArrayLike, y_pred: ArrayLike, | ||
self, y: ArrayLike, y_pred: ArrayLike, conformity_scores: ArrayLike |
There was a problem hiding this 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 mistake: conformity_scores
should not be an argument to get_signed_conformity_scores
.
self, | ||
y: NDArray, | ||
y_pred: NDArray, | ||
conformity_scores: NDArray, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to change the style (for consistency).
self, y: ArrayLike, y_pred: ArrayLike, | ||
self, | ||
y: ArrayLike, | ||
y_pred: ArrayLike, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to change the style (for consistency).
self, y: ArrayLike, y_pred: ArrayLike, | ||
self, | ||
y: ArrayLike, | ||
y_pred: ArrayLike, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to change the style (for consistency).
@@ -700,25 +724,41 @@ def predict( | |||
) | |||
) | |||
|
|||
if self.conformity_score_function_.compute_score_weights: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the motivation to modify the ConformityScore
class by adding a compute_score_weights
attribute and a get_weights
method.
I suggest to adapt the if
condition with a test like isinstance(self.conformity_score_function_, WeightedConformityScore)
. Is this possible in your opinion? Or am I missing something?
@@ -484,6 +485,29 @@ def _pred_multi(self, X: ArrayLike) -> NDArray: | |||
y_pred_multi = self._aggregate_with_mask(y_pred_multi, self.k_) | |||
return y_pred_multi | |||
|
|||
def _quantile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A description would be appreciated (even if the method name is explicit).
""" | ||
Returns the indices that would sort the array. | ||
In case of ties, breaks them randomly. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A detailed description would be appreciated (with Parameters
, Returns
, Raises
and Examples
sections if relevant).
""" | ||
Calculates the weighted empirical quantile of `values`. | ||
Converted to Python from https://github.com/ryantibs/conformal/ | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A detailed description would be appreciated (with Parameters
, Returns
, Raises
and Examples
sections if relevant).
Thanks for the feedback! I will try to address the comments over the weekend. :) |
Description
Add covariate shift conformal method (https://arxiv.org/abs/1904.06019) to MAPIE for regression.
Fixes #72
Type of change
Please remove options that are irrelevant.
How Has This Been Tested?
Reproducing the results of the original paper.
Checklist
make lint
make type-check
make tests
make coverage
make doc