Feature/narwhalify arb imp #390
Conversation
|
I think one of the other issues is coming from here: Should be: |
|
Note, also:
|
| impute_value: float | str, | ||
| columns: str | list[str], | ||
| **kwargs: dict[str, bool], | ||
| impute_value: Union[int, float, str, bool], |
There was a problem hiding this comment.
Q: just checking we're happy to now accept bool values in impute_value? Only because it looks like that wasn't allowed before
There was a problem hiding this comment.
yeah I think this should be OK, i imagine it was just left out by oversight previously, unless you can think of a reason to exclude?
We have a test that makes sure the falsey value False works, so one potential issue covered there
There was a problem hiding this comment.
No reason I can think of to be fair, and like you said the test is a good check for it. I've also just noticed the docstring for this will need updating to include bool. Currently it's impute_value : int or float or str
| ): | ||
| transformer.transform(df) | ||
|
|
||
| @pytest.mark.parametrize("library", ["pandas", "polars"]) |
There was a problem hiding this comment.
Q: strangely there are a couple of the pandas tests here where the transformer doesn't appear to work properly. With the parameters (pandas, 'a', 'String', 'z') the transformer doesn't impute the value z, it just leaves it as null. And then for (pandas, 'c', 'Boolean', True) it actually appears to impute with False. Can you double check you're seeing the same thing?
There was a problem hiding this comment.
The problem being the tests pass because the dtype has still been preserved
Narwhalification of the arbitrary imputer #315