Although there are quite some changes that are PEP8 related and not directly related to this PR, LGTM, if tests pass.

Although there are quite some changes that are PEP8 related and not directly related to this PR, LGTM, if tests pass.

Thanks. Indeed, as per the description above, I addressed all formatting issues in the modified files so that autopep8/flake8 checks passed before submitting the PR.

The core parts of this PR seem okay, when I can find them

Thanks for reviewing,

OK, I could revert the formatting changes if that would be preferred (your first two comments above)?
Regarding the modification in exceptions.py please see my response above.

@agamemnonc Could you revert the style changes. I can make a review then.

@agamemnonc Could you revert the style changes. I can make a review then.

OK, done; @glemaitre please review.

Of course, there are now some flake8 warnings on the modified files (mostly due to long lines).

I think that we have some missing occurrences:
EDIT: sorry I did not checkout the good PR

I also think that we should change the common test:

with something like:

@ignore_warnings
def check_estimators_unfitted(name, estimator_orig):
    """Check that predict raises an exception in an unfitted estimator.

    Unfitted estimators should raise a NotFittedError.
    """

    # Common test for Regressors, Classifiers and Outlier detection estimators
    X, y = _boston_subset()

    estimator = clone(estimator_orig)

    msg = ("{} instance is not fitted yet. Call 'fit' with appropriate "
           "arguments".format(estimator.__class__.__name__))
    for method in ('decision_function', 'predict', 'predict_proba',
                   'predict_log_proba'):
        if getattr(estimator, method, None) is not None:
            assert_raises_regex(NotFittedError, msg,
                                getattr(estimator, method), X)

@jnothman Do you know why we were checking AttributeError and ValueError instead of directly NotFittedError?

Regarding the tests, I would propose to change the following:

@pytest.mark.parametrize('name', FOREST_ESTIMATORS)
def test_unfitted_feature_importances(name):
    err_msg = ('This {} instance is not fitted yet. Call 'fit' with appropriate '
               'arguments before using this method.'.format(name))
    pytest.raises(NotFittedError, match=err_msg):
         gettattr(FOREST_ESTIMATORS[name](), 'feature_importances')

Even if we don't touch a public API, I would add an entry in the what's new since we modified the tests and error message.

I think that we have some missing occurrences:
EDIT: sorry I did not checkout the good PR

I also think that we should change the common test:

scikit-learn/sklearn/utils/estimator_checks.py

Lines 1586 to 1616 in fdf2f38

def check_estimators_unfitted(name, estimator_orig):
"""Check that predict raises an exception in an unfitted estimator.

 Unfitted estimators should raise either AttributeError or ValueError. 
 The specific exception type NotFittedError inherits from both and can 
 therefore be adequately raised for that purpose. 
 """ 

 # Common test for Regressors, Classifiers and Outlier detection estimators 
 X, y = _boston_subset() 

 estimator = clone(estimator_orig) 

 msg = "fit" 

 if hasattr(estimator, 'predict'): 
     assert_raise_message((AttributeError, ValueError), msg, 
                          estimator.predict, X) 

 if hasattr(estimator, 'decision_function'): 
     assert_raise_message((AttributeError, ValueError), msg, 
                          estimator.decision_function, X) 

 if hasattr(estimator, 'predict_proba'): 
     assert_raise_message((AttributeError, ValueError), msg, 
                          estimator.predict_proba, X) 

 if hasattr(estimator, 'predict_log_proba'): 
     assert_raise_message((AttributeError, ValueError), msg, 
                          estimator.predict_log_proba, X) 

with something like:

@ignore_warnings
def check_estimators_unfitted(name, estimator_orig):
    """Check that predict raises an exception in an unfitted estimator.

    Unfitted estimators should raise a NotFittedError.
    """

    # Common test for Regressors, Classifiers and Outlier detection estimators
    X, y = _boston_subset()

    estimator = clone(estimator_orig)

    msg = ("{} instance is not fitted yet. Call 'fit' with appropriate "
           "arguments".format(estimator.__class__.__name__))
    for method in ('decision_function', 'predict', 'predict_proba',
                   'predict_log_proba'):
        if getattr(estimator, method, None) is not None:
            assert_raises_regex(NotFittedError, msg,
                                getattr(estimator, method), X)

This is now fixed. thanks!

You can use getattr(estimator, method, None) is not None: to get the default to None.
It avoids the try ... except

Yes, that's what the code currently looks like:

msg = ("{} instance is not fitted yet. Call 'fit' with appropriate "
       "arguments".format(estimator.__class__.__name__))

for method in ('decision_function', 'predict', 'predict_proba',
               'predict_log_proba'):
    if getattr(estimator, method, None) is not None:
        assert_raises_regex(NotFittedError, msg,
                            getattr(estimator, method), X)

The problem is I am not too sure how to include the deprecation here, i.e. cover the case where an Attribute or ValueError is raised instead with the previous error message ("fit"), in order to allow that and issue a DeprecationWarning.

Oh I see why you wanted a try except then. Basically use pytest.raises and check https://docs.pytest.org/en/latest/assert.html

I think something around:

with pytest.raises(NotFittedError) as excinfo:
    getattr(estimator, method), X)

if not str(excinfo.value) in msg and 'fit' in str.excinfo.value):
    # raise deprecation waring
else:
    assert 'fit' in str.excinfo.value

I wrote this pretty quickly. That might be buggy

Please resolve conflicts with master.

When you get around to resolving my comments, please also move your change log entry to v0.22.rst as version 0.21 has been released.

@jnothman GaussianProcessRegressor can work without calling fit. I added a tag requires_fit but I am not sure if it would be something that we want.

WDYT?

I think we have explicitly tried to support GPs without fit. Otherwise you upset all the bayesians.

And do you think that having the tag requires_fit (default to True) is a good idea?

@jnothman could you have another look. Apart of the tag I think that the PR is good to be merged.

Thanks @NicolasHug for reviewing.

I think I have now addressed the issues you raised, otherwise please let me know.

Thanks @agamemnonc

Thanks @agamemnonc

Thank you @glemaitre for your input and everyone else for reviewing.