Coverage decreased (-0.0%) to 95.1% when pulling 0ab2751 on larsmans:notfitted-base into 1c7f6f4 on scikit-learn:master.

fine with me

LGTM as well. Maybe @amueller you want to have a look as well.

What about the warnings though. Could also argue that it's part of the public API as it's possible to catch them (although probably a less common idiom than for an exception).

I think all the warnings should then be in the same place too. otherwise LGTM.

Could we have a file sklearn/exceptions.py for all warnings and exceptions?

I like @ragv's suggestion. WDYT @larsmans?

I thought of adding a sklearn.exceptions, but I didn't think it worth the effort to introduce a new module. base isn't big and I don't suppose we want to build large hierarchies of exceptions and warnings.

Both options seem fine to me.

No strong opinions. Although to be fair I think that sklearn.base is better suited to host generic base and mixin classes.

I think using a dedicated module to host our custom exceptions and warnings will help make our project organization more readable to newcomers.

I don't suppose we want to build large hierarchies of exceptions and warnings.

That sounds legit... :)

@GaelVaroquaux Would you mind sharing your views on this?

Merge? Has +1 from @agramfort @ogrisel and me as far as I can tell.

you need to rebase

The thing is that we now have a released version that tells users to import from utils.validation, so this has become a backwards incompatible change. I'm not even sure it's worthwhile anymore.

Meh. I'm moving ChangedBehaviorWarning here #4309. We should really be clear about what is public in utils. I think I'm +1 with a deprecation.

+1 with deprecating the exceptions and warnings and moving them to an officially public module that is not under sklearn.utils.validation.

• +0 for using sklearn.base
• +1 for introducing sklearn.exceptions & sklearn.warnings to be even more explicit.

Could we simplify and group warnings and exceptions both under exceptions.py as done in sympy?

Yes but not under sklearn.utils.exceptions, as the public / private status of the content of sklearn.utils is still in flux. sklearn.exceptions would be fine to make it explicit that this is part of the public API.

sklearn.exceptions would be fine to make it explicit that this is part of the public API.

Fair enough!

+1

@larsmans Would you mind if I took over this and added a sklearn.exceptions to group all the custom exception and warning classes?
(There is FitFailedWarning in cross_validation.py which could be put inside the proposed exceptions.py)

Bump for the sprint.

I was asked to share my opinion:

+1 for sklearn/exceptions. With regards to hierarchy: let's try and keep it as simple as possible for now, elsewhere it will take ages to merge.

At the sprint, we are taking this one over.

+1 for sklearn/exceptions.

@GaelVaroquaux You might want to review #4826 ;)

This can be closed, as #4826 has been merged.