Skip to content

Cleanup of simple_imputer #346

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Dec 1, 2021
Merged

Cleanup of simple_imputer #346

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
61b1a29
cleanup of simple_imputer
eddiebergman Nov 30, 2021
bbabad8
Fixed doc and typo
eddiebergman Nov 30, 2021
c92d039
Fixed docs
eddiebergman Nov 30, 2021
60b9194
Made changes, added test
eddiebergman Dec 1, 2021
7a3e792
Fixed init statement
eddiebergman Dec 1, 2021
5490604
Fixed docs
eddiebergman Dec 1, 2021
e790e71
Flake'd
eddiebergman Dec 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 104 additions & 56 deletions ...Torch/pipeline/components/preprocessing/tabular_preprocessing/imputation/SimpleImputer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
from typing import Any, Dict, List, Optional, Union

from ConfigSpace.configuration_space import ConfigurationSpace
from ConfigSpace.hyperparameters import (
CategoricalHyperparameter
)
from ConfigSpace.hyperparameters import CategoricalHyperparameter

import numpy as np

Expand All @@ -15,92 +13,142 @@


class SimpleImputer(BaseImputer):
"""
Impute missing values for categorical columns with '!missing!'
(In case of numpy data, the constant value is set to -1, under
the assumption that categorical data is fit with an Ordinal Scaler)
"""An imputer for categorical and numerical columns
Impute missing values for categorical columns with 'constant_!missing!'
Note:
In case of numpy data, the constant value is set to -1, under the assumption
that categorical data is fit with an Ordinal Scaler.
"""

def __init__(self,
random_state: Optional[Union[np.random.RandomState, int]] = None,
numerical_strategy: str = 'mean',
categorical_strategy: str = 'most_frequent'):
def __init__(
self,
random_state: Optional[Union[np.random.RandomState, int]] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We removed integer type from random_state, so it must be Optional[np.random.RandomState]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will do. As an fyi, this can cause different output if you run the same function twice on the same object. The random state produces a sequence of numbers.

For example, if you create a single RandomState object and pass it to every object that requires a random_state, you will get different output depending on the order in which objects use that random_state. On the flip-side, if you use an int, they are independant of each other and so it doesn't matter which order objects use it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that is also true. My explanation was not sufficient, but we, in reality, decided to use seed for int and random_state for np.random.RandomState.
So it is a very good decision if we switch to seed instead of random_state in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We kept random_state as the arg name and allow for both, internally we just pass the seed argument given at construction of an AutoSklearnClassifier so that internally it's an int passed throughout.

We follow sklearn in principle so we copy their expected behaviour.

numerical_strategy: str = 'mean',
categorical_strategy: str = 'most_frequent'
):
"""
Note:
Using 'constant' defaults to fill_value of 0 where 'constant_!missing!'
uses a fill_value of -1. This behaviour should probably be fixed.
Args:
random_state (Optional[Union[np.random.RandomState, int]]):
The random state to use for the imputer.
numerical_strategy (str: default='mean'):
The strategy to use for imputing numerical columns.
Can be one of ['mean', 'median', 'most_frequent', 'constant', 'constant_!missing!']
categorical_strategy (str: default='most_frequent')
The strategy to use for imputing categorical columns.
Can be one of ['mean', 'median', 'most_frequent', 'constant_zero']
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can move this doc-string to the enums once we make them.

Btw, choices of categorical_strategy is incompatible with the hyperparamer search space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm just not sure what you mean by the second point, should I do something with respect to this?

Copy link
Collaborator

@nabenabe0928 nabenabe0928 Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

we have the following as the default searching space, right?
So I think either this value_range or your doc-string should follow the other.
I am assuming that you took the doc-string from AutoSKLearn, so probably this value_range should be replaced, don't you think?

        categorical_strategy: HyperparameterSearchSpace = HyperparameterSearchSpace(
            hyperparameter='categorical_strategy',
            value_range=("most_frequent", "constant_!missing!"),
            default_value="most_frequent"
        )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh my bad, I see your point. I've updated the docstring rather than update the search space, just to be inline with what's there so the actual functionality doesn't change.

I actually looked at the sklearn docs for possible values

"""
super().__init__()
self.random_state = random_state
self.numerical_strategy = numerical_strategy
self.categorical_strategy = categorical_strategy

def fit(self, X: Dict[str, Any], y: Any = None) -> BaseImputer:
"""
The fit function calls the fit function of the underlying model
and returns the transformed array.
def fit(self, X: Dict[str, Any], y: Optional[Any] = None) -> BaseImputer:
""" Fits the underlying model and returns the transformed array.
Args:
X (np.ndarray): input features
y (Optional[np.ndarray]): input labels
X (np.ndarray):
The input features to fit on
y (Optional[np.ndarray]):
The labels for the input features `X`
Returns:
instance of self
SimpleImputer:
returns self
"""
self.check_requirements(X, y)
categorical_columns = X['dataset_properties']['categorical_columns'] \
if isinstance(X['dataset_properties']['categorical_columns'], List) else []
if len(categorical_columns) != 0:

# Choose an imputer for any categorical columns
categorical_columns = X['dataset_properties']['categorical_columns']

if isinstance(categorical_columns, List) and len(categorical_columns) != 0:
if self.categorical_strategy == 'constant_!missing!':
self.preprocessor['categorical'] = SklearnSimpleImputer(strategy='constant',
# Train data is numpy
# as of this point, where
# Ordinal Encoding is using
# for categorical. Only
# Numbers are allowed
# fill_value='!missing!',
fill_value=-1,
copy=False)
# Train data is numpy as of this point, where an Ordinal Encoding is used
# for categoricals. Only Numbers are allowed for `fill_value`
imputer = SklearnSimpleImputer(strategy='constant', fill_value=-1, copy=False)
self.preprocessor['categorical'] = imputer
else:
self.preprocessor['categorical'] = SklearnSimpleImputer(strategy=self.categorical_strategy,
copy=False)
numerical_columns = X['dataset_properties']['numerical_columns'] \
if isinstance(X['dataset_properties']['numerical_columns'], List) else []
if len(numerical_columns) != 0:
imputer = SklearnSimpleImputer(strategy=self.categorical_strategy, copy=False)
self.preprocessor['categorical'] = imputer

# Choose an imputer for any numerical columns
numerical_columns = X['dataset_properties']['numerical_columns']

if isinstance(numerical_columns, List) and len(numerical_columns) > 0:
if self.numerical_strategy == 'constant_zero':
self.preprocessor['numerical'] = SklearnSimpleImputer(strategy='constant',
fill_value=0,
copy=False)
imputer = SklearnSimpleImputer(strategy='constant', fill_value=0, copy=False)
self.preprocessor['numerical'] = imputer
else:
self.preprocessor['numerical'] = SklearnSimpleImputer(strategy=self.numerical_strategy, copy=False)
imputer = SklearnSimpleImputer(strategy=self.numerical_strategy, copy=False)
self.preprocessor['numerical'] = imputer

return self

@staticmethod
def get_hyperparameter_search_space(
dataset_properties: Optional[Dict[str, BaseDatasetPropertiesType]] = None,
numerical_strategy: HyperparameterSearchSpace = HyperparameterSearchSpace(hyperparameter='numerical_strategy',
value_range=("mean", "median",
"most_frequent",
"constant_zero"),
default_value="mean",
),
numerical_strategy: HyperparameterSearchSpace = HyperparameterSearchSpace(
hyperparameter='numerical_strategy',
value_range=("mean", "median", "most_frequent", "constant_zero"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better if we can use Enum.
We did not have it so much just because I did not join the refactoring phase.
At least, I would like to reduce so many dependencies on hard-coded strings as much as possible.
Because this file also uses so many hard-coded strings, which we can avoid by enum.

from enum import Enum
from typing import List


class NumericalImputerChoices(Enum):
    mean = "mean"
    median = "median"
    most_frequent = "most_frequent"
    constant_zero = "constant_zero"

    @classmethod
    def get_choices(cls) -> List[str]:
        return [c.value for c in cls]


class CategoricalImputerChoices(Enum):
    most_frequent = "most_frequent"
    constant_missing = "constant_!missing!"

    @classmethod
    def get_choices(cls) -> List[str]:
        return [c.value for c in cls]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, you do however require to update these whenever sklearn updates them as well as that is where they are forwarded to.

Copy link
Contributor Author

@eddiebergman eddiebergman Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I can see why you want to do that but it makes the code that uses it a little less pretty. For example, it's hard to see where the error with the follow code is:

numerical_strategy: HyperparameterSearchSpace = HyperparameterSearchSpace(
    hyperparameter='numerical_strategy',
    value_range=NumericalImputerChoice.get_choices(),
    default_value=NumericalImputerChoice.mean,
),

It should be default_value=NumericalImputerChoice.mean.value. The problem here is that Enum's are namespaced i.e.

  • NumericalImputerChoice.mean == "NumericalImputerChoice.mean"

... where as we would like

  • NumericalImputerChoice.mean == "mean".

This is more readily achievable with a NamedTuple, meaning no classes or anything are required. From my Java days, this was similar and in general, enum values should never be directly used, such as their string value, and rather more as flags.

I will implement the Enum version and you can change it if you like or leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another clean'ish solution is just ditch the enum part.

class Choices:
    x: str = "x"
    y: str = "y"
    
assert Choices.x = "x"

The type is still a string, meaning it's easy to use, people don't need to know about the existence of this class to use it, they can still pass a string. It also allows for the internal code to use the class, where we do know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As another note, it also makes parameters extremely long, it's 101 characters which is over the character count limit of the checker:

    def __init__(
        self,
        random_state: Optional[np.random.RandomState] = None,
        numerical_strategy: NumericalImputerChoice = NumericalImputerChoice.mean.value,
        categorical_strategy: CategoricalImputerChoice = CategoricalImputerChoice.most_frequent.value
    ):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not changed it for now, there's too many decisions to make that I think can be addressed based on how you would like it done yourself. I've changed it back to strings.

Copy link
Contributor

@ravinkohli ravinkohli Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hey, thanks for the insights. Yeah, I think such changes should be a part of a separate PR as this PR is meant to clean up the messy statements in this file. Also, it would be better if the hyperparameter strings in all the components are consistent so it would require changing a lot of files which I think is beyond the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have the same thing, there is Literal at the moment but the type checking for it doesn't work as expected.

def f(s: Literal['yes', 'no'])
   ...
   
f("yes") # Type Error

param: Literal = "yes"
f(param) # Okay

Copy link
Collaborator

@nabenabe0928 nabenabe0928 Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

All the problems come from the fact that we still need to follow python3.7.
Once we switched to python3.8, we can use Literal...

in general, enum values should never be directly used, such as their string value

Yeah, I agree with you. I am just uncomfortable using software that assumes that users google string choices (typically sklearn).
And I found it really useful to be able to use enum or class such as in numpy, facebook ax..
But I got your point, let's stay for now.
I did not know some points you mentioned, thanks for raising them. @eddiebergman .

(just a question, but) the better solution will be something like this:

class NumericalImputerChoices(NamedTuple):
    mean = "mean"
    median = "median"

num_imputer_choices = NumericalImputerChoices()

and use str for all the typings.

Btw, you do not need to put .value (FYI).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, you can add python after the backticks ``` of a code block to get python highlighting ;)

default_value="mean",
),
categorical_strategy: HyperparameterSearchSpace = HyperparameterSearchSpace(
hyperparameter='categorical_strategy',
value_range=("most_frequent",
"constant_!missing!"),
default_value="most_frequent")
value_range=("most_frequent", "constant_!missing!"),
default_value="most_frequent"
)
) -> ConfigurationSpace:
"""Get the hyperparameter search space for the SimpleImputer
Args:
dataset_properties (Optional[Dict[str, BaseDatasetPropertiesType]])
Properties that describe the dataset
Note: Not actually Optional, just adhering to its supertype
numerical_strategy (HyperparameterSearchSpace: default = ...)
The strategy to use for numerical imputation
caterogical_strategy (HyperparameterSearchSpace: default = ...)
The strategy to use for categorical imputation
Returns:
ConfigurationSpace
The space of possible configurations for a SimpleImputer with the given
`dataset_properties`
"""
cs = ConfigurationSpace()
assert dataset_properties is not None, "To create hyperparameter search space" \
", dataset_properties should not be None"
if len(dataset_properties['numerical_columns']) \
if isinstance(dataset_properties['numerical_columns'], List) else 0 != 0:

if dataset_properties is None:
raise ValueError("SimpleImputer requires `dataset_properties` for generating"
" a search space.")

if (
isinstance(dataset_properties['numerical_columns'], List)
and len(dataset_properties['numerical_columns']) != 0
):
add_hyperparameter(cs, numerical_strategy, CategoricalHyperparameter)

if len(dataset_properties['categorical_columns']) \
if isinstance(dataset_properties['categorical_columns'], List) else 0 != 0:
if (
isinstance(dataset_properties['categorical_columns'], List)
and len(dataset_properties['categorical_columns'])
):
add_hyperparameter(cs, categorical_strategy, CategoricalHyperparameter)

return cs

@staticmethod
def get_properties(dataset_properties: Optional[Dict[str, BaseDatasetPropertiesType]] = None
) -> Dict[str, Union[str, bool]]:
def get_properties(
dataset_properties: Optional[Dict[str, BaseDatasetPropertiesType]] = None
) -> Dict[str, Union[str, bool]]:
"""Get the properties of the SimpleImputer class and what it can handle
Returns:
Dict[str, Union[str, bool]]:
A dict from property names to values
"""
return {
'shortname': 'SimpleImputer',
'name': 'Simple Imputer',
Expand Down