Cleanup of simple_imputer

[eddiebergman]

Does a clean-up of SimpleImputer stemming from the weirdness of the if statements, mentioned in #345. Does not change functionality.

• Simplifies the fit method a bit
• Add docstrings
• Fixes some typing
• Document unexpected functionality of passing 'constant' for categorical_strategy instead of 'constant_!missing!'.
  • 'constant' - fill_value = 0 (sklearn default)
  • 'constant_!missing!' - fill_value = -1

[codecov]

Codecov Report

Merging #346 (e790e71) into development (ef8d21a) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           development     #346      +/-   ##
===============================================
+ Coverage        82.11%   82.21%   +0.10%     
===============================================
  Files              153      153              
  Lines             8812     8817       +5     
  Branches          1341     1529     +188     
===============================================
+ Hits              7236     7249      +13     
+ Misses            1107     1103       -4     
+ Partials           469      465       -4     

Impacted Files	Coverage Δ
.../tabular_preprocessing/imputation/SimpleImputer.py	100.00% <100.00%> (ø)
autoPyTorch/ensemble/ensemble_builder.py	73.16% <0.00%> (+0.83%)	⬆️
...peline/components/training/trainer/base_trainer.py	96.82% <0.00%> (+1.05%)	⬆️
...ipeline/components/setup/network_backbone/utils.py	88.63% <0.00%> (+1.51%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef8d21a...e790e71. Read the comment docs.

[ravinkohli]

Thanks for the PR. I think it can be merged now.

[nabenabe0928]

Hello thanks for the PR.
Since we had some changes (probably those are not in ASK), so I will leave some comments.

[nabenabe0928]

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

[eddiebergman]

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.

[nabenabe0928]

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.

[eddiebergman]

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.

[nabenabe0928]

Hi thanks for cleaning up this file.
It is much better than the previous state:)
I left some comments as well.

[nabenabe0928]

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]

[eddiebergman]

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.

[eddiebergman]

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.

[eddiebergman]

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.

[eddiebergman]

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
    ):

[eddiebergman]

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.

[ravinkohli]

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.

[eddiebergman]

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

[nabenabe0928]

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).

[eddiebergman]

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

[nabenabe0928]

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.

[eddiebergman]

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

[nabenabe0928]

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"
        )

[eddiebergman]

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