Coverage remained the same when pulling 79a620b on ragv:partial_fit_testing_3896 into 4f1c381 on scikit-learn:master.

That seems a nice list (although testing that models are the same currently is done by testing the output of predict/transform).

I'm not altogether certain about the fit then partial_fit semantics: fit does not necessarily leave the model in a state in which incremental training can occur. I do think that we should decide what correct semantics are.

What is the current semantics?
Should fit always leave the model in a state where partial_fit is possible?

I am stuck with the 2nd alone... The case when fit then partial_fit == partial_fit then partial_fit does not seem to be consistently true.

( assuming that the equality checks for the accuracy ( score) )

Kindly review my current implementation of the other tests.

You should probably use predict/transform directly rather than score, as do the existing invariance tests.

I think fit, partial fit == partial_fit, partial_fit is the wrong semantics. I'd rather fit, partial fit => exception just to be clear.

You're assuming all partial_fit estimators are predictors. Some are or may be transformers. Perhaps we should use something like:

def _assert_same_model_method(method, X, estimator1, estimator2, msg):
    if hasattr(estimator1, method):
        assert hasattr(estimator2, method), '{!r} has transform, but {!r} does not'.format(estimator1, estimator2)
        assert_array_almost_equal(getattr(estimator1, method)(X), getattr(estimator2, method)(X), 2, 'When testing {}: {}'.format(method, msg))


def assert_same_model(X, estimator1, estimator2, msg):
    _assert_same_model_method('predict')
    _assert_same_model_method('transform')

@jnothman partial_fit after a fit does not raise an exception... Is it probably a bug?

For your 2nd comment, thanks a lot for the review... I'll make the changes :)

Should fit always leave the model in a state where partial_fit is possible?

@amueller I'm not quite sure. I think it should either raise an error or it should overwrite and do a new fit from the beginning, if one calls partial_fit after fit. If partial_fit starts from where fit left, then it does not make a difference whether you fit or partial_fit, right? ;)

@MechCoder @amueller From what I observed fit always restarts the estimator. partial_fit continues from the previous setup irrespective of whether the previous setup was a fit or a partial_fit.

But as you and others suggested, perhaps partial_fit after fit should raise an exception?

We could either.

1. Raise an exception.
2. Or overwrite the previously fit model if partial_fit is being called after fit or fit is being called after partial_fit.

I'm inclined towards the second one but I'm not sure what is the best thing to do.

I feel 2b is already implemented... I guess its between 2a or 1... Anyway thanks for your comments :)

A code revision related question... When revisions are suggested to the code, am I expected to add a new commit reflecting those revision or do I (soft) reset my previous work, make the changes and recommit?

I think, you can do it either way. But if those revisions are minor, and if you feel it worthless to add a new commit for those (like pep8, cosmits and stuff), you can just amend your commit and push by force (at least, that's what I do).

Thanks... I'll also follow the same :)

The point is that the interaction between fit and partial_fit hasn't been previously addressed. The assumption is that the user will choose one or the other, but not both. Forcing a particular behaviour by way of the proposed tests will make the matter more consistent, but is also perhaps more of a framework than is actually needed, because the case is somewhat pathological.

@jnothman To your earlier comment,

Perhaps we should use something like {code}

Could you expand a little by what you meant?

I understood the part where you said all those estimators should not be tested assuming they are predictors and that we should separately test them based on whether they are a Transformer or Predictor ...

But I fail to understand the two code snippets...
If you meant that we could filter out Transformer like estimators which implement partial_fit... I just modified the method_filter to accept ! as a prefix to the method names...

[ @amueller perhaps such a functionality could be useful for type_filter too? ]

So for estimators that are not Transformers:
ests = all_estimators(method_filter=["predict", "!transform", "partial_fit"])

And for Transformers with partial_fit capability :
ests = all_estimators(method_filter=["transform", "partial_fit"], type_filter="transformer")
# The type filter is perhaps redundant here

Excuse me if I understood your comment incorrectly...

@amueller I added some tests for all_estimators function as discussed here. Kindly take a look when you find time... I've also added ! prefixing for type_filter and method_filter....

( The pfit tests are WIP though... )

I am tempted to ask for a check similar to the one I did in #4535 [edit] for just ensuring that fitting twice gives the same model. It seems that currently gives "interesting" results.

@amueller Could I do that in #4162 ? :) after this gets merged :)

I think arrays should be compared using array_almost_equal not array_equal

Noted Thanks :))

I'm not entirely convinced of the usefulness of a general assert_not_same_model helper. When would you use that? For the partial_fit tests, can't you just test that predict yields different results?

When would you use that? For the partial_fit tests, can't you just test that predict yields different results?

If you know your estimator is a predictor, then yes. The assert_{not_}same_model helpers should apply to whatever extent possible to dictionary learning, IPCA, MBKMeans, Birch... While a bit hacky given that we don't officially have a notion of model equality in scikit-learn API, I think this sort of helper has the potential to clean up a number of common tests.

I'd be interested in seeing these helpers moved to a smaller PR, like #4162, so that it can be reviewed and merged without a large change to the tests. Then work on merging this PR, and refactoring existing tests to use the helpers can begin.

Yes that will be really better! I'll cherry pick the helpers into a new branch and raise a PR!!

Not sure what's the state of this PR at the moment, but please consider #5416.
It would be nice to have a common test over all estimators with partial_fit to check whether two subsequent calls to fit (not partial) behave well. For example:

est.fit(X)
# Different input shape. It may raise errors in the case the internal state is not reset
est.fit(X[, :-1])
# It should give the same result as the first line
est.fit(X) 

In the case of scalers, the issue came from the fact that we are now implementing fit with one call to a partial_fit, without a reset first.

Ping @lesteve

@giorgiop Work is going on at #4841, where new assert helpers to compare models is being written... Once that gets over fit reset tests will go in and later we'll have this PR in (hopefully) :)

Is this PR just waiting for review or is there still work to be done?

This is gonna need a fresh start if we're going to do it. And we'd need to do significant work on the semantics of partial_fit.