Skip to content

Conversation

@Dmatryus
Copy link
Collaborator

@Dmatryus Dmatryus commented Apr 21, 2025

  1. I have placed the Analyzer in a separate class from which all Executors of this type are inherited. This made it possible to slightly reduce code duplication and make the class schema more intuitive and homogeneous.
  2. Change calc function in Calculator for more intuitive using.

…rs of this type are inherited. This made it possible to slightly reduce code duplication and make the class schema more intuitive and homogeneous.
@Dmatryus Dmatryus changed the title Dev/analyzers refactor Global refactor Apr 21, 2025
Copy link
Collaborator

@TonyKatkov89 TonyKatkov89 left a 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,
Copy link
Collaborator

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

Comment on lines 459 to 485


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)
Copy link
Collaborator

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
@tikhomirovd
Copy link
Collaborator

Loving the new imports — super clean!
Will take a closer look at the rest soon.

TonyKatkov89
TonyKatkov89 previously approved these changes Aug 28, 2025
Copy link
Collaborator

@TonyKatkov89 TonyKatkov89 left a comment

Choose a reason for hiding this comment

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

ಥ_ಥ

@Dmatryus Dmatryus dismissed TonyKatkov89’s stale review October 7, 2025 16:36

The merge-base changed after approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants