Skip to content

[MRG] Deprecate any import that is not from sklearn.neural_network #14913

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

Closed
wants to merge 11 commits into from
Closed

[MRG] Deprecate any import that is not from sklearn.neural_network #14913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
838ee72
renamed files with leading underscore
NicolasHug Sep 6, 2019
d005f90
comments and pep8
NicolasHug Sep 6, 2019
a9895f9
again
NicolasHug Sep 6, 2019
c850d8e
without conflicts this time
NicolasHug Sep 6, 2019
b4360c3
Merge branch 'lol' into underscore_files_neural_network
NicolasHug Sep 6, 2019
970f65c
remove extra line break
NicolasHug Sep 8, 2019
a4ca778
update message
NicolasHug Sep 8, 2019
8bc7660
change warning msg
NicolasHug Sep 9, 2019
6286843
synteax error
NicolasHug Sep 9, 2019
fc86b67
better comment
NicolasHug Sep 9, 2019
97a8841
[doc build]
NicolasHug Sep 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,13 @@ def pytest_runtest_setup(item):
def pytest_runtest_teardown(item, nextitem):
if isinstance(item, DoctestItem):
set_config(print_changed_only=False)


# We don't want pytest to run these files since they immediately raise a
# DeprecationWarning and that would make CI unhappy.
# These files don't contain any real code, they just raise a warning. We still
# ensure these warnings are properly raised in the tests.
collect_ignore_glob = [
"sklearn/neural_network/rbm.py", # 0.24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this also result in the doctests not to run?

Copy link
Member

@rth rth Sep 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, maybe rather not raise the warning in a pytest session?

Something like ,

import sys
if not getattr(sys, '_is_pytest_session', False):
    warnings.warn(...)

in the affected files, where sys._is_pytest_session is already set in conftest.py

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no docstring in rbm.py. There's almost no code in these files now, all they do is raises a warning.

@rth I have no strong opinion but I think that we should rather have pytest ignore a file, rather than have a file have specific behavior for pytest. LMK.

Copy link
Member

@rth rth Sep 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no docstring in rbm.py. There's almost no code in these files now, all they do is raises a warning.

For now. Hard to to know some time later that this file is ignored by pytest just by looking at it (or to remove that line once the file is removed). I think if we can do deprecations locally without touching global config files it is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now

I don't see the file evolving, ever? This is the file we just created.

We only need to ignore the file because we run the tests with -Werror::DeprecationWarning. Not raising the warning when we are in a pytest session is treating the issue in the reverse order IMO, but OK. I'll address it in #14939 if you don't mind

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the file evolving, ever? This is the file we just created.

Me neither, but who knows. Accidents and messed up merge conflicts do happen.

Not raising the warning when we are in a pytest session is treating the issue in the reverse order IMO, but OK.

Yes, I agree it's not ideal, but both this and changing the conftest.py is a way around our non standard use of error on warning. Not sure if there is a better solution.

"sklearn/neural_network/multilayer_perceptron.py", # 0.24
]
6 changes: 3 additions & 3 deletions sklearn/neural_network/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@

# License: BSD 3 clause

from .rbm import BernoulliRBM
from ._rbm import BernoulliRBM

from .multilayer_perceptron import MLPClassifier
from .multilayer_perceptron import MLPRegressor
from ._multilayer_perceptron import MLPClassifier
from ._multilayer_perceptron import MLPRegressor

__all__ = ["BernoulliRBM",
"MLPClassifier",
Expand Down
Loading