-
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 Adaptive Conformal Inference method for Time Series #341
Add Adaptive Conformal Inference method for Time Series #341
Conversation
Optimisation méthode ACI, update chaque pas du gamma
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.
@QMaphan, I offer you a quick first review for your contribution for scores applied to regression. All suggestions will improve the style and visibility of the code.
I'd also suggest adapting your score so that it's called with the following signature: (y_true, y_interval) instead of (y_true, y_low, y_up) .
alpha = alpha_np | ||
return alpha | ||
|
||
def adapt_conformal_inference( |
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.
def adapt_conformal_inference( | |
def _adapt_conformal_inference( |
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.
Why not, but then we'd have to replace partial_fit
with _partial_fit
.
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.
no because it would be a breaking changes (cf my comment about partial_fit)
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.
In this case, I prefer to leave it as is.
@@ -96,45 +112,27 @@ def _relative_conformity_scores( | |||
) | |||
return scores | |||
|
|||
def fit( | |||
def partial_fit( |
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.
If I well understood your PR, now if method == "enbpi" then you can either call .update
or .partial_fit
. For now maybe we can keep both, but if we want to keep only the .update
we could add a DepreciationWarning to partial_fit
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.
For the moment, I have no intention of withdrawing it.
self, | ||
X: ArrayLike, | ||
y: ArrayLike, | ||
ensemble: bool = False, |
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.
shoudnl't we keep the same value of ensemble
for all the class ?
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 agree, but we also have the same problem with sym
in this case. I'm doing this to reproduce the results.
Co-authored-by: Vincent Blot <[email protected]>
@@ -633,6 +633,8 @@ def predict( | |||
X: ArrayLike, | |||
ensemble: bool = False, | |||
alpha: Optional[Union[float, Iterable[float]]] = None, | |||
optimize_beta: bool = False, |
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.
why this change ?
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.
Because of function signature: https://stackoverflow.com/questions/51003146/python-3-6-signature-of-method-incompatible-with-super-type-class
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.
Some last small changes but overall this PR will go a long way for MAPIE!
Description
Adaptive Conformal Inference for time series with general dependency.
Fixes #334
Fixes #390
Type of change
How Has This Been Tested?
Checklist
make lint
make type-check
make tests
make coverage
make doc