-
Notifications
You must be signed in to change notification settings - Fork 130
chore(skore): Switch from mypy to ty
#2461
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
base: main
Are you sure you want to change the base?
Changes from 17 commits
4682aeb
686ad08
80f2a47
6f68e4d
596623e
db756f7
4b319ee
f74c67c
cc5d105
b20810b
8acdaf7
254c4ef
3db1306
5194a6b
395ad2b
b01008a
8a02169
9b733ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,12 @@ | |
| from dataclasses import InitVar, dataclass, field, fields | ||
| from datetime import datetime, timezone | ||
| from math import isfinite | ||
| from typing import TYPE_CHECKING, Literal, cast | ||
| from typing import TYPE_CHECKING, Any, Literal, cast | ||
|
|
||
| from joblib import hash | ||
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Generator | ||
| from typing import Any | ||
|
|
||
| from skore import CrossValidationReport, EstimatorReport | ||
|
|
||
|
|
@@ -118,10 +117,14 @@ def metric(report: EstimatorReport, name: str) -> float | None: | |
|
|
||
| return cast_to_float(getattr(report.metrics, name)(data_source="test")) | ||
|
|
||
| def __post_init__(self, report: EstimatorReport) -> None: # type: ignore[override] | ||
| def __post_init__(self, report: EstimatorReport | CrossValidationReport) -> None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know it violates the Liskov substitution principle, but your change is not true. Please revert.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can fix the violation in a dedicated issue as we have done in https://github.com/probabl-ai/skore/blob/main/skore-hub-project/src/skore_hub_project/report/estimator_report.py, but it is not important here. |
||
| """Initialize dynamic fields.""" | ||
| super().__post_init__(report) | ||
|
|
||
| from skore import EstimatorReport | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is redundant with the import in
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I got test failures when this wasn't imported out of TYPE_CHECKING. So it looks like |
||
|
|
||
| report = cast(EstimatorReport, report) | ||
|
|
||
| self.rmse = self.metric(report, "rmse") | ||
| self.log_loss = self.metric(report, "log_loss") | ||
| self.roc_auc = self.metric(report, "roc_auc") | ||
|
|
@@ -164,10 +167,14 @@ def timing(report: CrossValidationReport, label: str) -> float | None: | |
|
|
||
| return cast_to_float(series.iloc[0]) | ||
|
|
||
| def __post_init__(self, report: CrossValidationReport) -> None: # type: ignore[override] | ||
| def __post_init__(self, report) -> None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert. |
||
| """Initialize dynamic fields.""" | ||
| super().__post_init__(report) | ||
|
|
||
| from skore import CrossValidationReport | ||
|
|
||
| report = cast(CrossValidationReport, report) | ||
|
|
||
| self.rmse_mean = self.metric(report, "rmse") | ||
| self.log_loss_mean = self.metric(report, "log_loss") | ||
| self.roc_auc_mean = self.metric(report, "roc_auc") | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
This change has to be discussed, as
tydoesn't have a strict mode for now and we would to be strict as much as possible inskore-hub-projectandskore-local-project.https://docs.astral.sh/ty/reference/typing-faq/#does-ty-have-a-strict-mode
Could you please list me the rules that will not be applied with this change?