8000 [MRG+3] move NotFittedError to sklearn.base by larsmans · Pull Request #4309 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG+3] move NotFittedError to sklearn.base #4309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[MRG+3] move NotFittedError to sklearn.base #4309

wants to merge 1 commit into from

Conversation

larsmans
Copy link
Member
@larsmans larsmans commented Mar 1, 2015

This moves the recently added NotFittedError to sklearn.base so user code that wants to catch it does not have to import it from sklearn.utils.validation. I'm not sure if we've made a decision regarding the public status of (parts of) sklearn.utils yet, but in any case, an exception that is thrown at the user is a public thing, and it is not a utility.

Ping @ogrisel.

@larsmans larsmans changed the title MAINT move NotFittedError to sklearn.base [MRG] move NotFittedError to sklearn.base Mar 1, 2015
@coveralls
Copy link

Coverage Status

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

@agramfort
Copy link
Member

fine with me

@ogrisel
Copy link
Member
ogrisel commented Mar 1, 2015

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).

@amueller
Copy link
Member
amueller commented Mar 1, 2015

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

@raghavrv
Copy link
Member
raghavrv commented Mar 1, 2015

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

@ogrisel
Copy link
Member
ogrisel commented Mar 2, 2015

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

@larsmans
Copy link
Member Author
larsmans commented Mar 2, 2015

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.

@amueller
Copy link
Member
amueller commented Mar 2, 2015

Both options seem fine to me.

@ogrisel
Copy link
Member
ogrisel commented Mar 2, 2015

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.

@raghavrv
Copy link
Member

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?

@amueller
Copy link
Member

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

@glouppe glouppe changed the title [MRG] move NotFittedError to sklearn.base [MRG+3] move NotFittedError to sklearn.base May 13, 2015
@agramfort
Copy link
Member

you need to rebase

@larsmans
Copy link
Member Author

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.

@amueller amueller mentioned this pull request May 13, 2015
Merged
@amueller
Copy link
Member

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.

@ogrisel
Copy link
Member
ogrisel commented May 21, 2015

+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.

@raghavrv
Copy link
Member

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

@ogrisel
Copy link
Member
ogrisel commented May 21, 2015

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.

@raghavrv
Copy link
Member

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

Fair enough!

@amueller
Copy link
Member

+1

@raghavrv
Copy link
Member
raghavrv commented Jun 5, 2015

@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)

@raghavrv raghavrv mentioned this pull request Jun 5, 2015
Merged
@glouppe
Copy link
Contributor
glouppe commented Oct 19, 2015

Bump for the sprint.

@GaelVaroquaux
Copy link
Member

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.

@raghavrv
Copy link
Member

+1 for sklearn/exceptions.

@GaelVaroquaux You might want to review #4826 ;)

@glouppe
Copy link
Contributor
glouppe commented Oct 20, 2015

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

@glouppe glouppe closed this Oct 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@larsmans @coveralls @agramfort @ogrisel @amueller @raghavrv @glouppe @GaelVaroquaux
0