BUG fix SparseCoder to follow scikit-learn API and allow cloning

[sdpython]

Reference Issues/PRs

Fixes#8675
Fixes#16336
Supersedes and closes#16346

What does this implement/fix? Explain your changes.

SparseCoder cannot be used in a GridSearchCV because it could not be cloned. A parameter in the constructor had a different meaning when stored in an instance (dictionary).

[glemaitre]

Uhm did something about this one. Let me check.

[glemaitre]

Found it: #16346

[sdpython]

Let me know which one you prefer. PR #16346 does not test cloning and that's what caused the issue. I recommend adding one more test.

[glemaitre]

I think this is important to make the estimator scikit-learn compliant (it will crash soon otherwise). Cloning is part of that so we should merge both PR. Could you merge my PR inside yours like this you supersede mine? I will review it then.

[glemaitre]

I just solve the conflicts in mine so you should have no so much trouble to merge them.

[cmarmo]

@sdpython do you mind editing the PR adding that you will also close #16336? Thanks for your patience.

[sdpython]

ok :)

[glemaitre]

@sdpython I will do a review. I think that we might need to add some of the test (usually done in common test) but that require specific shape for X and dictionary.

[ogrisel]

A few comments but otherwise LGTM. I don't think there is an easy way to re-add SparseCoder to test_common as it requires to specify the dictionary in the constructor params and the dictionary shape should be consistent with X.shape[1].

[glemaitre]

We would need to have something like that (it is only the transformer).

deftest_sparse_coder_common_transformer(): fromfunctoolsimportpartialfromsklearn.utils.estimator_checksimportcheck_transformer_data_not_an_arrayfromsklearn.utils.estimator_checksimportcheck_transformer_generalfromsklearn.utils.estimator_checksimportcheck_transformers_unfittedrng=np.random.RandomState(777) n_components, n_features=40, 3init_dict=rng.rand(n_components, n_features) sc=SparseCoder(init_dict) check_transformer_data_not_an_array(sc.__class__.__name__, sc) check_transformer_general(sc.__class__.__name__, sc) check_transformer_general_memmap=partial( check_transformer_general, readonly_memmap=True ) check_transformer_general_memmap(sc.__class__.__name__, sc) check_transformers_unfitted(sc.__class__.__name__, sc)

[glemaitre]

I think this is wiser to merge with the current test and see how the test_common.py could be modify such that the size of the dictionary can be modified on the fly.

[adrinjalali]

A few comments but otherwise LGTM. I don't think there is an easy way to re-add SparseCoder to test_common as it requires to specify the dictionary in the constructor params and the dictionary shape should be consistent with X.shape[1].

I have encountered this while working on other estimators outside sklearn, and it's partly why I wanted to have a better way of telling common tests now to generate required data for certain tests.

[sdpython]

deftest_sparse_coder_common_transformer(): fromfunctoolsimportpartialfromsklearn.utils.estimator_checksimportcheck_transformer_data_not_an_arrayfromsklearn.utils.estimator_checksimportcheck_transformer_generalfromsklearn.utils.estimator_checksimportcheck_transformers_unfittedrng=np.random.RandomState(777) n_components, n_features=40, 3init_dict=rng.rand(n_components, n_features) sc=SparseCoder(init_dict) check_transformer_data_not_an_array(sc.__class__.__name__, sc) check_transformer_general(sc.__class__.__name__, sc) check_transformer_general_memmap=partial( check_transformer_general, readonly_memmap=True ) check_transformer_general_memmap(sc.__class__.__name__, sc) check_transformers_unfitted(sc.__class__.__name__, sc)

Added.

[ogrisel]

LGTM! Thanks @sdpython!

[glemaitre]

There is a single thing missing which is an entry in what's new because we are deprecated one parameter.
Can you this acknowledging yourself @sdpython and then we are good for merging.

[agramfort]

@glemaitre merge if happy

thx @sdpython

[agramfort]

actually @glemaitre had approved so go !

thx @sdpython

[glemaitre]

I will make a PR adding the deprecation in what's new :)