-
Notifications
You must be signed in to change notification settings - Fork 2
Update the EnsembleDetector with multiple voting system
#114
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
Changes from 3 commits
24b2944
234810d
c22f787
7744b87
416dc1c
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 |
|---|---|---|
|
|
@@ -2,18 +2,16 @@ | |
| from apeiron.drift_detection.detectors.base import BaseDriftDetector | ||
|
|
||
|
|
||
| def load_drift_detector(cfg: Config) -> BaseDriftDetector: | ||
| """Dynamically load and instantiate a drift detector based on its name. | ||
| def _build_detector(detector_name: str, cfg: Config) -> BaseDriftDetector: | ||
|
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. The Config still should still hold the detector name in it. We should continue to derive detector name from the config instead of accepting it separately. It's slightly less error prone and a bit easier to use. Changing it to this means every caller will do: instead of:
Collaborator
Author
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. The load_drift_detection function is still using the detector name set in config. This change is necessary (i.e. splitting the load function into _build_detector that takes a given detector) to cover the which means we are calling _build_detector for each detector in the list.
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. Okay so for the ensemble detector, the name that we're passing to buld_detector will differ from the name in the config? That's what it looks like based on the below code snippet. Odd behavior but it's an internal only method so I suppose it's fine. detectors=[_build_detector(name, cfg) for name in sub_names] |
||
| """Instantiate a single (non-ensemble) drift detector from the config. | ||
|
|
||
| Args: | ||
| detector_name (str): Name of the drift detector class to load. | ||
| detector_name (str): Name of the drift detector class to build. | ||
| cfg: Configuration object containing parameters for the detector. | ||
|
|
||
| Returns: | ||
| BaseDriftDetector: An instance of the specified drift detector. | ||
| """ | ||
| detector_name = cfg.drift_detection.detector_name | ||
|
|
||
| detector_instance: BaseDriftDetector | ||
| if detector_name == "ADWINDetector": | ||
| from apeiron.drift_detection.detectors.statistical_detectors import ( | ||
|
|
@@ -52,19 +50,6 @@ def load_drift_detector(cfg: Config) -> BaseDriftDetector: | |
| ) | ||
|
|
||
| detector_instance = ModelPerformanceDetector() | ||
| elif detector_name == "EnsembleDetector": | ||
| raise NotImplementedError( | ||
| "EnsembleDetector requires configuration of sub-detectors, " | ||
| "which is not yet implemented. Use ADWINDetector, KSWINDetector, " | ||
| "PageHinkleyDetector, or ModelPerformanceDetector instead." | ||
| ) | ||
|
|
||
| # from apeiron.drift_detection.detectors.model_performance_detector import ( | ||
| # EnsembleDetector, | ||
| # ) | ||
|
|
||
| # detector_instance = EnsembleDetector() | ||
|
|
||
| elif detector_name == "EvalDetector": | ||
| from apeiron.drift_detection.detectors.model_performance_detector import ( | ||
| ModelEvalDetector, | ||
|
|
@@ -75,3 +60,37 @@ def load_drift_detector(cfg: Config) -> BaseDriftDetector: | |
| raise ValueError(f"Unknown drift detector: {detector_name}") | ||
|
|
||
| return detector_instance | ||
|
|
||
|
|
||
| def load_drift_detector(cfg: Config) -> BaseDriftDetector: | ||
| """Dynamically load and instantiate a drift detector based on its name. | ||
|
|
||
| Args: | ||
| cfg: Configuration object containing parameters for the detector. | ||
|
|
||
| Returns: | ||
| BaseDriftDetector: An instance of the specified drift detector. | ||
| """ | ||
| detector_name = cfg.drift_detection.detector_name | ||
|
|
||
| if detector_name != "EnsembleDetector": | ||
| return _build_detector(detector_name, cfg) | ||
|
|
||
| from apeiron.drift_detection.detectors.model_performance_detector import ( | ||
| EnsembleDetector, | ||
| ) | ||
|
|
||
| sub_names = cfg.drift_detection.ensemble_detectors | ||
| if not sub_names: | ||
| raise ValueError( | ||
| "EnsembleDetector requires [drift_detection] ensemble_detectors to list " | ||
| "at least one sub-detector, e.g. " | ||
| 'ensemble_detectors = ["ADWINDetector", "KSWINDetector"]' | ||
| ) | ||
| if "EnsembleDetector" in sub_names: | ||
| raise ValueError("EnsembleDetector cannot be nested inside itself") | ||
|
|
||
| return EnsembleDetector( | ||
| detectors=[_build_detector(name, cfg) for name in sub_names], | ||
| voting=cfg.drift_detection.ensemble_voting, | ||
| ) | ||
Uh oh!
There was an error while loading. Please reload this page.