-
Notifications
You must be signed in to change notification settings - Fork 239
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.
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
Implement & test handling of NaNs in preprocessing with handle_nans decorator #130
base: master
Are you sure you want to change the base?
Implement & test handling of NaNs in preprocessing with handle_nans decorator #130
Conversation
henrifroese
commented
Jul 30, 2020
•
edited
Loading
edited
- Decorator used everywhere it's necessary in preprocessing.py with appropriate fill values
- test_nan.py added with tests for the preprocessing module
- Decorator used everywhere it's necessary in preprocessing.py with appropriate fill values - test_nan.py added with test's for the preprocessing module Co-authored-by: Maximilian Krahn <maximilian.krahn@icloud.com>
Thank you for the PR Henri! I believe we are not dealing with nan in the correct "Pandas" way. The rule should be: Preprocessing: Example:
Where the Pandas-user expects:
Representation: Here we need to deal with such NA values. The problems arises when we pass NA values to scikit learn functions, right?
If that's not possible, your initial idea was to use a decorator as it might be useful. It keeps the code simple and does two things:
Personally, I'm not a fan of the decorator in this case as the processes are a bit hidden. Instead, we can simply have a function: My proposal is:
As you already spent quite a large amount of time, if you wish I can take care of this. P.s I'm sorry this is taking that long. It's surely my fault for not having explained this task properly as well as not having seen the issues initially. |
Hi @jbesomi I think we will leave it till Monday and have a quick talk about it then 🚀 |
Henri and Max, can you please just confirm you want me to take over this? |
Yes that would be great 👌 |