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 1 commit
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
88 changes: 37 additions & 51 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
Expand Up @@ -29,45 +29,36 @@ def __init__(
categorical_strategy: str = 'most_frequent'
):
"""
Parameters
----------
random_state: Optional[Union[np.random.RandomState, int]] = None
The random state to use for the imputer

numerical_strategy: str = 'mean',
The strategy to use for imputing numerical columns.
Can be one of ['mean', 'median', 'most_frequent', 'constant', 'constant_!missing!']

Note:
Using 'constant' defaults to fill_value of 0 where 'constant_!missing!'
uses a fill_value of -1. This behaviour should probably be fixed.

categorical_strategy: str = 'most_frequent'
The strategy to use for imputing categorical columns.
Can be one of ['mean', 'median', 'most_frequent', 'constant_zero']
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: Optional[Any] = None) -> BaseImputer:
"""
The fit function calls the fit function of the underlying model
and returns the transformed array.
""" Fits the underlying model and returns the transformed array.

Parameters
----------
X: np.ndarray
The input features to fit on
Args:
X (np.ndarray):
The input features to fit on
y (Optional[np.ndarray]):
The labels for the input features `X`

y: Optional[np.ndarray]
The labels for the input features `X`

Returns
-------
SimpleImputer
returns self
Returns:
SimpleImputer: returns the object itself
"""
self.check_requirements(X, y)

Expand Down Expand Up @@ -102,7 +93,7 @@ def get_hyperparameter_search_space(
dataset_properties: Optional[Dict[str, BaseDatasetPropertiesType]] = None,
numerical_strategy: HyperparameterSearchSpace = HyperparameterSearchSpace(
hyperparameter='numerical_strategy',
value_range=("mean", "median", "most_frequet", "constant_zero"),
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(
Expand All @@ -113,22 +104,19 @@ def get_hyperparameter_search_space(
) -> ConfigurationSpace:
"""Get the hyperparameter search space for the SimpleImputer

Parameters
----------
dataset_properties: Optional[Dict[str, BaseDatasetPropertiesType]] = None
Properties that describe the dataset

numerical_strategy: HyperparameterSearchSpace = HyperparameterSearchSpace(...)
The strategy to use for numerical imputation

caterogical_strategy: HyperparameterSearchSpace = HyperparameterSearchSpace(...)
The strategy to use for categorical imputation

Returns
-------
ConfigurationSpace
The space of possible configurations for a SimpleImputer with the given
`dataset_properties`
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()

Expand Down Expand Up @@ -156,10 +144,8 @@ def get_properties(
) -> 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
Returns:
Dict[str, Union[str, bool]]: A dict from property names to values
"""
return {
'shortname': 'SimpleImputer',
Expand Down