Conversation
There was a problem hiding this comment.
Pull request overview
This PR consolidates several previously scattered enums into octopus/types.py and updates the codebase (models, modules, study, predict, manager, tests) to import and use the unified enum definitions—especially around feature-importance configuration/labels and logging groups.
Changes:
- Centralise enums in
octopus/types.py(e.g.,LogGroup,ImputationMethod,ModelName,ResultType, and FI-related enums). - Update modules/models/predict/study/manager code to use the consolidated enums (including stricter FI typing via
FIType,FIComputeMethod,ShapExplainerType). - Adjust tests to use the new enums and updated error messaging from enum coercion.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/study/test_core.py | Updates imports to use ImputationMethod from octopus.types. |
| tests/predict/test_predict.py | Updates assertion to match enum-coercion ValueError message for invalid FIType. |
| tests/modules/test_module_result.py | Moves ResultType import to octopus.types. |
| tests/models/test_model_config.py | Updates ModelConfig.feature_method usage to FIComputeMethod. |
| tests/models/test_hyperparameter.py | Updates ModelConfig.feature_method usage to FIComputeMethod. |
| octopus/types.py | Adds consolidated enums for logging, models, results, and FI APIs. |
| octopus/study/types.py | Removes ImputationMethod definition (now referenced as moved). |
| octopus/study/core.py | Updates import of ImputationMethod to come from octopus.types. |
| octopus/predict/task_predictor_test.py | Types fi_type as `FIType |
| octopus/predict/task_predictor.py | Types fi_type as `FIType |
| octopus/predict/feature_importance.py | Types shap_type as `ShapExplainerType |
| octopus/modules/sfs/core.py | Updates FI result labelling and enum imports to consolidated types. |
| octopus/modules/roc/core.py | Moves ResultType import to octopus.types. |
| octopus/modules/rfe2/module.py | Refactors fi_method_rfe to FIComputeMethod with attrs converter/validator. |
| octopus/modules/rfe2/core.py | Uses FIComputeMethod for config + FIResultLabel for output labelling. |
| octopus/modules/rfe/core.py | Updates FI result labelling and enum imports to consolidated types. |
| octopus/modules/octo/training.py | Uses consolidated LogGroup, FIComputeMethod, ShapExplainerType, ModelName. |
| octopus/modules/octo/objective_optuna.py | Moves LogGroup/ModelName imports to octopus.types. |
| octopus/modules/octo/module.py | Types fi_methods_bestbag as list[FIComputeMethod] with conversion. |
| octopus/modules/octo/core.py | Moves LogGroup/ResultType imports to octopus.types. |
| octopus/modules/octo/bag.py | Uses FIComputeMethod for FI dispatch and consolidated LogGroup. |
| octopus/modules/mrmr/module.py | Refactors feature_importance_method to FIComputeMethod. |
| octopus/modules/mrmr/core.py | Maps FIComputeMethod config to FIResultLabel for result filtering. |
| octopus/modules/efs/core.py | Updates FI result labelling and enum imports to consolidated types. |
| octopus/modules/boruta/core.py | Updates FI result labelling and enum imports to consolidated types. |
| octopus/modules/base.py | Removes local ResultType/FI enums; uses ResultType from octopus.types. |
| octopus/modules/autogluon/core.py | Updates FI result labelling and enum imports to consolidated types. |
| octopus/modules/init.py | Re-exports consolidated enums (FIComputeMethod, FIDataset, FIResultLabel, ResultType). |
| octopus/models/regression_models.py | Updates feature_method to use FIComputeMethod. |
| octopus/models/model_name.py | Removes ModelName definition (now referenced as moved). |
| octopus/models/core.py | Imports ModelName from octopus.types. |
| octopus/models/config.py | Types ModelConfig.feature_method as FIComputeMethod with converter. |
| octopus/models/classification_models.py | Updates feature_method to use FIComputeMethod. |
| octopus/models/init.py | Re-exports ModelName from octopus.types. |
| octopus/manager/workflow_runner.py | Moves ResultType import to octopus.types. |
| octopus/manager/execution.py | Moves LogGroup import to octopus.types. |
| octopus/logger.py | Moves LogGroup definition out; imports from octopus.types. |
| octopus/datasplit.py | Moves LogGroup import to octopus.types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nihaase
left a comment
There was a problem hiding this comment.
Only some styling things.
2ba75c9 to
b40525d
Compare
dasmy
left a comment
There was a problem hiding this comment.
I really like the change because it makes things much more explicit, safe, and readable.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
octopus/modules/init.py:54
octopus.modulesused to exportFIMethod, but this PR replaces it withFIResultLabel. Sinceoctopus/modules/base.pystill definesFIMethod, removing the re-export is a breaking change forfrom octopus.modules import FIMethod. Consider adding a backward-compatible alias/export (e.g.FIMethod = FIResultLabel) and including it in__all__until a deprecation window has passed.
from octopus.types import FIDataset, FIResultLabel
from .base import ModuleExecution, Task
from .boruta import Boruta
from .context import StudyContext
from .efs import Efs
from .mrmr import Mrmr
from .octo import Octo
from .result import ModuleResult, ResultType
from .rfe import Rfe
from .rfe2 import Rfe2
from .roc import Roc
from .sfs import Sfs
from .utils import rdc_correlation_matrix
__all__ = [
"AutoGluon",
"Boruta",
"Efs",
"FIDataset",
"FIResultLabel",
"ModuleExecution",
"ModuleResult",
"Mrmr",
"Octo",
"ResultType",
"Rfe",
"Rfe2",
"Roc",
"Sfs",
"StudyContext",
"Task",
"rdc_correlation_matrix",
]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
dd2b9ee to
d096e8e
Compare
fbe7050 to
e940a5e
Compare
Fixes #350.
FI logic used raw strings like
"permutation", "shap", "internal"in model configs, modules, and the predict API. This made it easy to introduce typos and hard to discover which values were valid in a given context.What changed
MLType, LogGroup, ModelName, etc.) totypes.pyStrEnumtypes intypes.pyto replace FI related strings:FIResultLabel: column labels used in feature-importance result DataFrames (renamed fromFIMethod)FIComputeMethod: computation methods in model and module configs (used inModelConfig.feature_method, Octo.fi_methods_bestbag, Rfe2.fi_method_rfe, Mrmr.feature_importance_method, bag.py/training.py)FIType: thefi_typeparameter in the prediction API (TaskPredictor.calculate_fi)ShapExplainerType: theshap_typeparameter selecting between kernel, permutation, and exact SHAP explainersStrEnumand existing code that passes plain strings keeps workingclassification_models.py / regression_models.pyto useFIComputeMethodbag.py, training.py, feature_importance.py, mrmr/core.py, rfe2/core.py)to compare against enumsWhat's next
This is the first refactoring for str/literals to enum conversion, other types will follow in a second PR.