Exposing conversion to Float32 in the Cleaner - #1440
Conversation
|
I'm wondering if converting to float32 should be an option for the config manager #1427 |
| datetime_format : str, default=None | ||
| The format to use when parsing dates. If None, the format is inferred. | ||
|
|
||
| convert_float32 : bool, default=False |
There was a problem hiding this comment.
Here we are using a different API as in the TableVectorizer (and as in a few other objects in scikit-learn that do type conversions, I believe).
I understand why, but the differences are quite large (different parameter name, different type of accepted value), and it will make it hard for the user to memorize. Can we get closer to the canonical API?
There was a problem hiding this comment.
eg we could have numerical_dtype="float32"
There was a problem hiding this comment.
here the option is either "convert to float32", or "keep as is", if we use numerical_dtype="float32", what would be the other option?
I am open to changing the name of the parameter, but given that in this case it's really an on-off switch I think a boolean flag makes the most sense
There was a problem hiding this comment.
do you have an example of the scikit-learn objects?
There was a problem hiding this comment.
For OneHotEncoder and others, we usually have a dtype parameter (https://scikit-learn.org/stable/modules/generated/sklearn.preprocessing.OneHotEncoder.html) to control the output dtype.
There was a problem hiding this comment.
something like numerical_dtype=None by default when numbers are not converted and numerical_dtype="float32" when the conversion must be done?
tbh I'm still not convinced this is better than having boolean flag here
|
here the option is either "convert to float32", or "keep as is", if we use numerical_dtype="float32", what would be the other option?
"None", I would expect, to do nothing.
|
|
something like numerical_dtype=None by default when numbers are not converted and numerical_dtype="float32" when the conversion must be done?
Yes
tbh I'm still not convinced this is better than having boolean flag here
It's about minimizing the specificities of each object: they should have a similar look and feel (like school uniforms :D )
|
|
ready for review |
|
|
||
| vectorizer = Cleaner(numerical_dtype="float32") | ||
| out = vectorizer.fit_transform(X) | ||
| assert sbd.is_float(out["float"]) |
There was a problem hiding this comment.
Does this check for float, or float32?
There was a problem hiding this comment.
good catch, it only checks for float
There was a problem hiding this comment.
so I just realized that this test, like all other tests in table_vectorizer, is not testing for pandas, pandas-nullable-types and polars
I'll do a quick fix to get this test through, but we'll have to update all tests
|
I'll do a quick fix to get this test through, but we'll have to update all tests
Yes, let's just worry about the Float32 functionality in this PR and address the issue that you mention here in another PR.
|
|
just a small comment, I think in skrub we have been using "numeric" and not "numericAL" everywhere so it would be nice to stay consistent |
4 participants
Fixes #1439