Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds an ML module to the HypEx framework, introducing advanced covariate adjustment methods for variance reduction in A/B testing. It includes CUPED (Controlled Experiments Using Pre-Experiment Data) and CUPAC (Covariate-Updated Pre-Analysis Correction) implementations.
Key changes:
- Added DataGenerator class with support for time correlations and various distributions
- Implemented CUPEDTransformer for classic covariate adjustment
- Created CUPACExecutor with automatic model selection for advanced adjustment
- Updated ABTest class to support both CUPED and CUPAC parameters
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| hypex/utils/tutorial_data_creation.py | Added DataGenerator class for synthetic data with time correlations and covariate structures |
| hypex/utils/enums.py | Added "ml" enum value for ML-related experiment data |
| hypex/transformers/cuped.py | New CUPEDTransformer for variance reduction using pre-experiment covariates |
| hypex/ml/cupac.py | New CUPACExecutor with automatic model selection for advanced covariate adjustment |
| hypex/ab.py | Updated ABTest to support CUPED and CUPAC parameters with mutual exclusion |
| hypex/experiments/base.py | Modified experiment execution to properly handle transformer sequences |
Comments suppressed due to low confidence (1)
hypex/ab.py:1
- There are extra closing parentheses on lines 90 and 91. The first closing parenthesis on line 90 closes the Multitest constructor, but there's an additional closing parenthesis on line 91 that doesn't have a matching opening parenthesis.
from __future__ import annotations
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hypex/ml/cupac.py
Outdated
| def __init__( | ||
| self, | ||
| cupac_features: Dict[str, list], | ||
| key: Any = "", | ||
| models: Optional[Dict[str, Any]] = None, | ||
| n_folds: int = 5, | ||
| random_state: Optional[int] = None, | ||
| ): | ||
| super().__init__(target_role=TargetRole(), key=key) | ||
| self.cupac_features = cupac_features | ||
| self.models = models | ||
| self.n_folds = n_folds | ||
| self.random_state = random_state | ||
| self.best_model = None | ||
| self.best_model_name = None | ||
| self.best_score = None | ||
| self.variance_reduction = None | ||
| self.feature_importances_ = None | ||
| self.is_fitted = False | ||
| self.model_results_ = {} | ||
|
|
There was a problem hiding this comment.
This __init__ method is duplicated at lines 31-46. The class has two identical initialization methods which will cause the second one to override the first, leading to inconsistent behavior.
| def __init__( | |
| self, | |
| cupac_features: Dict[str, list], | |
| key: Any = "", | |
| models: Optional[Dict[str, Any]] = None, | |
| n_folds: int = 5, | |
| random_state: Optional[int] = None, | |
| ): | |
| super().__init__(target_role=TargetRole(), key=key) | |
| self.cupac_features = cupac_features | |
| self.models = models | |
| self.n_folds = n_folds | |
| self.random_state = random_state | |
| self.best_model = None | |
| self.best_model_name = None | |
| self.best_score = None | |
| self.variance_reduction = None | |
| self.feature_importances_ = None | |
| self.is_fitted = False | |
| self.model_results_ = {} |
hypex/experiments/base.py
Outdated
| if self.transformer: | ||
| experiment_data = deepcopy(data) | ||
| for transformer in self.transformer: | ||
| experiment_data = transformer.execute(experiment_data) | ||
| else: | ||
| experiment_data = data | ||
|
|
||
|
|
There was a problem hiding this comment.
The code assumes self.transformer is iterable, but the Experiment class constructor accepts transformer: bool | None. This will cause a TypeError when trying to iterate over a boolean value.
| if self.transformer: | |
| experiment_data = deepcopy(data) | |
| for transformer in self.transformer: | |
| experiment_data = transformer.execute(experiment_data) | |
| else: | |
| experiment_data = data | |
| experiment_data = deepcopy(data) if self.transformer else data | |
| for transformer in self.transformer: | |
| experiment_data = transformer.execute(experiment_data) |
hypex/transformers/cuped.py
Outdated
| @@ -0,0 +1,49 @@ | |||
|
|
|||
There was a problem hiding this comment.
The file starts with an empty line which is unnecessary and inconsistent with other files in the codebase.
hypex/ml/cupac.py
Outdated
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
The file starts with three empty lines which is unnecessary and inconsistent with other files in the codebase.
hypex/ml/__init__.py
Outdated
| @@ -1,3 +1,5 @@ | |||
|
|
|||
There was a problem hiding this comment.
The file starts with an empty line which is unnecessary and inconsistent with other files in the codebase.
anathema-git
force-pushed
the
ml_module
branch
from
d05b81a to
934d550
Compare
No description provided.