Check arguments of MissingIndicator imputer when handling sparse arrays
#13240
Conversation
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
Could you add an entry in what's new
|
ok. I fixed all the issues that came up in the review. Most importantly,
|
|
I think it's fine now. But maybe it would be more readable to separate the case sparse input + missing = 0, i.e. have a test which checks everything works on normal inputs and another one which checks that an error is raised otherwise. |
|
@jeremiedbb that makes sense, but I would need to skip over some input combinations. Otherwise, I couldn't use pytest decorators. How would you do that? |
|
I means that we need to specified the combination in a single parametrize instead of creating several of them and make the product.
If there is no too much case I agree with jerimie
Sent from my phone - sorry to be brief and potential misspell.
|
|
I would remove the missing=0 case from the current test and add a test with missing=0. |
|
ok. I enumerated all possible combinations not involving missing_values == 0 and sparse inputs. For the latter case, I added a new test. |
| - |Fix| Fixed a bug in :class:`linear_model.LassoLarsIC`, where user input | ||
| ``copy_X=False`` at instance creation would be overridden by default | ||
| parameter value ``copy_X=True`` in ``fit``. | ||
| parameter value ``copy_X=True`` in ``fit``. |
There was a problem hiding this comment.
Please avoid fixing pep8 issues not related to your PR. It makes reviews harder to follow
There was a problem hiding this comment.
Ok. I will avoid it for future PRs, but I guess I should keep this one.
There was a problem hiding this comment.
we usually ask contributors to revert it :/
|
LGTM. Will merged when CI are green. |
|
thx @btel |
|
my first every scikit-learn PR merged 🎊 Thanks @jeremiedbb @glemaitre @jnothman @agramfort for help! |
|
🍺 Thanks a lot. |
…rays (scikit-learn#13240) * adding argument check for the case spare=True and missing_values=0 issue scikit-learn#12750 * missing indicator: handle case array is sparse and missing values==0 issue scikit-learn#12750 * fix flake8 issues * remove empty lines * remove sparse==True and missing_value==0 case; handle `transform` method * remove blankline * separate out test for missing_value==0 and sparse input * remove spurious blank lines * added what's new entry * fixing lint issues * remove redundant code * fix issue number in "what's new" * move duplicate code to _validate_input * fix test for transform method * remove nested if conditions
…parse arrays (scikit-learn#13240)" This reverts commit 40bdf83.
…parse arrays (scikit-learn#13240)" This reverts commit 40bdf83.
…rays (scikit-learn#13240) * adding argument check for the case spare=True and missing_values=0 issue scikit-learn#12750 * missing indicator: handle case array is sparse and missing values==0 issue scikit-learn#12750 * fix flake8 issues * remove empty lines * remove sparse==True and missing_value==0 case; handle `transform` method * remove blankline * separate out test for missing_value==0 and sparse input * remove spurious blank lines * added what's new entry * fixing lint issues * remove redundant code * fix issue number in "what's new" * move duplicate code to _validate_input * fix test for transform method * remove nested if conditions
5 participants
Reference Issues/PRs
Fixes issue #12750
What does this implement/fix? Explain your changes.
This PR implements a simple check of the arguments to make sure that the MissingIndicator raises an exception when both arguments
sparse=Trueandmissing_values=0or array is sparse andmissing_values=0. It fixes old tests that allowed for these cases and implements one new test.Any other comments?