-
Notifications
You must be signed in to change notification settings - Fork 8
Global refactor #165
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: master
Are you sure you want to change the base?
Global refactor #165
Conversation
…rs of this type are inherited. This made it possible to slightly reduce code duplication and make the class schema more intuitive and homogeneous.
TonyKatkov89
left a comment
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.
significance in MDE should be rechecked
| data: Dataset, | ||
| test_data: Dataset | None = None, | ||
| significance: float = 0.95, | ||
| significance: float = 0.05, |
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.
Either this should remain 0.95 or we need to change the way how "m" calculated
hypex/comparators/abstract.py
Outdated
|
|
||
|
|
||
| class PowerTesting(Comparator, ABC): | ||
| def __init__( | ||
| self, | ||
| grouping_role: ABCRole | None = None, | ||
| significance: float = 0.95, | ||
| power: float = 0.8, | ||
| key: Any = "", | ||
| ): | ||
| super().__init__( | ||
| compare_by="groups", | ||
| grouping_role=grouping_role, | ||
| key=key, | ||
| ) | ||
| self.significance = significance | ||
| self.power = power | ||
|
|
||
| @classmethod | ||
| @abstractmethod | ||
| def calc( | ||
| cls, data: Dataset, test_data: Dataset | None = None, **kwargs: float | ||
| ) -> float: | ||
| pass | ||
|
|
||
| def execute(self, data: ExperimentData) -> ExperimentData: | ||
| return super().execute(data) |
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 to move PowerTesting class to abstract comparator file?
And ruff + isort
|
Loving the new imports — super clean! |
TonyKatkov89
left a comment
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.
ಥ_ಥ
The merge-base changed after approval.
Uh oh!
There was an error while loading. Please reload this page.