FIX bug #462: Decoupling infinite interval production and quantile calculation #463
Conversation
There was a problem hiding this comment.
Hey @thibaultcordier,
I think the PR is great! Thank you for including the examples! This PR could be approved as is, but there are some small changes that I think could help make it clearer. Especially in the example, I don't think it's needed to add the prefit_asym part.
Thank you!
|
|
||
|
|
||
| ############################################################################## | ||
| # Experiment 2: Again but without fixing random_state |
There was a problem hiding this comment.
I would only keep the experiment 2 where you fix the random_state.
There was a problem hiding this comment.
I have to keep this part, because the reader will no longer understand why in the next plot, the different methods no longer overlap when they did before.
| @@ -412,6 +412,29 @@ def check_gamma( | |||
| ) | |||
|
|
|||
|
|
|||
| def get_effective_calibration_samples(scores: NDArray, sym: bool): | |||
There was a problem hiding this comment.
Is the sym not by default True?
There was a problem hiding this comment.
The sym is by default True in ConformityScore. In any case, we need to compute the effective calibration sample size to compute the quantile.
| @@ -369,6 +384,11 @@ def get_bounds( | |||
|
|
|||
| By default ``False``. | |||
|
|
|||
| allow_infinite_bounds: bool | |||
There was a problem hiding this comment.
Why do we call it allow_infinite_bounds and the unbounded?
There was a problem hiding this comment.
In MapieRegressor, it is helpful to keep allow_infinite_bounds to understand what it is. In get_bounds, it is not helpful to understand the code because too long.
|
|
||
|
|
||
| ############################################################################## | ||
| # Experiment 4: Extensive experimentation on different delta and n_calib |
There was a problem hiding this comment.
I would only show the reference, prefit.
There was a problem hiding this comment.
It is important to show the asymmetric case to avoid any misunderstanding/publication of an issue that is not an issue.
|
|
||
|
|
||
| ############################################################################## | ||
| # Section 2: Comparison with different MAPIE CP methods |
There was a problem hiding this comment.
Not sure that this example adds a lot of insights. I would make this example shorter by getting rid of the prefit_asym part.
There was a problem hiding this comment.
In practice, this part could be improved by adding more MAPIE methods (like split, CV+, CQR etc). In any case, we let the user test it themselves.
Description
Decoupling infinite interval production and quantile calculation
Fixes #462
Type of change
How Has This Been Tested?
Checklist
make lintmake type-checkmake testsmake coveragemake doc