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

UPD: Migration for MapieRegressor #305

Merged
merged 27 commits into from
Jun 7, 2023

Conversation

thibaultcordier
Copy link
Collaborator

@thibaultcordier thibaultcordier commented May 4, 2023

Description

Propose a migration of some methods / classes / modules.

Type of change

  • New feature = Migration (non-breaking change which adds functionality)

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

@thibaultcordier thibaultcordier self-assigned this May 4, 2023
@thibaultcordier thibaultcordier linked an issue May 4, 2023 that may be closed by this pull request
@thibaultcordier thibaultcordier added this to In progress in Developments via automation May 4, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f1055e5) 100.00% compared to head (76f3f00) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #305   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        32    +5     
  Lines         3475      3523   +48     
  Branches       360       364    +4     
=========================================
+ Hits          3475      3523   +48     
Impacted Files Coverage Δ
mapie/__init__.py 100.00% <ø> (ø)
mapie/utils.py 100.00% <ø> (ø)
mapie/conformity_scores/__init__.py 100.00% <100.00%> (ø)
mapie/conformity_scores/conformity_scores.py 100.00% <100.00%> (ø)
...ie/conformity_scores/residual_conformity_scores.py 100.00% <100.00%> (ø)
mapie/quantile_regression.py 100.00% <100.00%> (ø)
mapie/regression/__init__.py 100.00% <100.00%> (ø)
mapie/regression/quantile_regression.py 100.00% <100.00%> (ø)
mapie/regression/regression.py 100.00% <100.00%> (ø)
mapie/regression/time_series_regression.py 100.00% <100.00%> (ø)
... and 6 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@thibaultcordier thibaultcordier added the enhancement New feature or request label May 5, 2023
@thibaultcordier thibaultcordier changed the title UPD: Migration & Refactorization for MapieRegressor UPD: Migration for MapieRegressor May 5, 2023
@thibaultcordier thibaultcordier marked this pull request as ready for review May 25, 2023 07:54
Developments automation moved this from In progress to Review approved Jun 6, 2023
@thibaultcordier thibaultcordier merged commit b99bb8c into master Jun 7, 2023
11 checks passed
Developments automation moved this from Review approved to Done Jun 7, 2023
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.

Looks good to me, I think there are some missing small things. But overall, will really simplify structure of MAPIE!

) == y``
It should be specified if ``consistency_check==True``.

By default ``np.float64(1e-8)``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that we use this as an eps in other classes in MAPIE.

from ._machine_precision import EPSILON

I think we should keep this uniform.

self,
sym: bool,
consistency_check: bool = True,
eps: np.float64 = np.float64(1e-8),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same logic for epsilon.

) -> NDArray:
"""
Placeholder for ``get_signed_conformity_scores``.
Subclasses should implement this method!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if we want to use exclamation marks. Seems a bit overdone ;)


Parameters
----------
y: NDArray
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this might be recurrent, but in the parameters, you call this an NDArray but it's an ArrayLike. Also you are not give the shape of these arrays.

"are not consistent. "
"The following equation must be verified: "
"self.get_estimation_distribution(y_pred, "
"self.get_conformity_scores(y, y_pred)) == y. " # noqa: E501
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this # noqa: E501?

def _check_predicted_data(
self,
y_pred: ArrayLike,
) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

No docstring?

def _all_strictly_positive(
self,
y: ArrayLike,
) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

No docstring?

@@ -0,0 +1,343 @@
from __future__ import annotations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Difficult to see with the github diff, but have you changed any of the code from the previous code to this one?

@@ -0,0 +1,709 @@
from __future__ import annotations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Difficult to see with the github diff, but have you changed any of the code from the previous code to this one?


y_pred_1, y_pis_1 = mapie_c1.predict(X, alpha=0.1)
y_pred_2, y_pis_2 = mapie_c2.predict(X, alpha=0.1)
np.testing.assert_allclose(y_pis_1[:, 0, 0], y_pis_2[:, 0, 0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we using np.testing.assert_allclose and not np.testing. assert_array_equal?

Developments automation moved this from Done to Review in progress Jun 7, 2023
@thibaultcordier thibaultcordier moved this from Review in progress to Done in Developments Jul 11, 2023
@thibaultcordier thibaultcordier deleted the 300-premise-adaptative-conformal-prediction branch September 20, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants