[WIP] Resolve #157 - Allow NaNs in Random*Samplers #167
Conversation
|
We can check what are the thoughts from the scikit-learn community. In fact, I am not a big fan to let @chkoar Can you open an issue to get a feedback? |
@glemaitre Actually I agree with that. I was thinking that we should be consistent with all samplers. I saw the ticket opened and tried to solve it. We could just label the issue as |
As you suggested, I would see what are the thoughts in scikit-learn community. If we find out that it will not change because they want to keep it not flexible, we can close for that reason and document the issue. |
|
How should we handle this PR? scikit-learn/scikit-learn#7737 |
|
I would not include this for the moment |
|
I would not include this for the moment [2] |
|
I still think that we could try this #157 (comment) like in Decision Trees in scikit-learn (https://github.com/scikit-learn/scikit-learn/blob/a5ab948/sklearn/tree/tree.py#L380) |
With this PR I tried to address the #157 issue, but doing this the
check_estimatortest is falling because internally calls thecheck_estimators_nan_infwhich desn't acceptNaNsif name not in ['Imputer']and raises error.We could skip the two tests that failing for this reason and open an issue in the
scikit-learnrepository to get feedback like this scikit-learn/scikit-learn#6981. Thoughts @dvro @glemaitre ?