Skip to content
Merged
Changes from all commits
Commits
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
69 changes: 66 additions & 3 deletions doc/developers/contributing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -790,17 +790,35 @@ In the following example, k is deprecated and renamed to n_clusters::

import warnings

def example_function(n_clusters=8, k=None):
if k is not None:
def example_function(n_clusters=8, k='not_used'):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should wait and see if others think if this is a good idea. @jnothman @ogrisel @rth ?

if k != 'not_used':
warnings.warn("'k' was renamed to n_clusters in version 0.13 and "
"will be removed in 0.15.", DeprecationWarning)
n_clusters = k

When the change is in a class, we validate and raise warning in ``fit``::

import warnings

class ExampleEstimator(BaseEstimator):
def __init__(self, n_clusters=8, k='not_used'):
self.n_clusters = n_clusters
self.k = k

def fit(self, X, y):
if k != 'not_used':
warnings.warn("'k' was renamed to n_clusters in version 0.13 and "
"will be removed in 0.15.", DeprecationWarning)
self._n_clusters = k
else:
self._n_clusters = self.n_clusters

As in these examples, the warning message should always give both the
version in which the deprecation happened and the version in which the
old behavior will be removed. If the deprecation happened in version
0.x-dev, the message should say deprecation occurred in version 0.x and
the removal will be in 0.(x+2). For example, if the deprecation happened
the removal will be in 0.(x+2), so that users will have enough time to
adapt their code to the new behaviour. For example, if the deprecation happened
in version 0.18-dev, the message should say it happened in version 0.18
and the old behavior will be removed in version 0.20.

Expand All @@ -812,6 +830,51 @@ same information as the deprecation warning as explained above. Use the
``k`` was renamed to ``n_clusters`` in version 0.13 and will be removed
in 0.15.

What's more, a deprecation requires a test which ensures that the warning is
raised in relevant cases but not in other cases. The warning should be caught
in all other tests (using e.g., ``@pytest.mark.filterwarnings``),
and there should be no warning in the examples.


Change the default value of a parameter
---------------------------------------

If the default value of a parameter needs to be changed, please replace the
default value with a specific value (e.g., ``warn``) and raise
``FutureWarning`` when users are using the default value. In the following
example, we change the default value of ``n_clusters`` from 5 to 10
(current version is 0.20)::

import warnings

def example_function(n_clusters='warn'):
if n_clusters == 'warn':
warnings.warn("The default value of n_clusters will change from "
"5 to 10 in 0.22.", FutureWarning)
n_clusters = 5

When the change is in a class, we validate and raise warning in ``fit``::

import warnings

class ExampleEstimator:
def __init__(self, n_clusters='warn'):
self.n_clusters = n_clusters

def fit(self, X, y):
if self.n_clusters == 'warn':
warnings.warn("The default value of n_clusters will change from "
"5 to 10 in 0.22.", FutureWarning)
self._n_clusters = 5

Similar to deprecations, the warning message should always give both the
version in which the change happened and the version in which the old behavior
will be removed. The docstring needs to be updated accordingly. We need a test
which ensures that the warning is raised in relevant cases but not in other
cases. The warning should be caught in all other tests
(using e.g., ``pytest.mark.filter_warnings``), and there should be no warning
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@glemaitre Should we use @pytest.mark.filterwarnings here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@aboucaud I've pushed to master, please refer the latest doc :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes but you forgot to remove the _ :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, thanks a lot @aboucaud :)

in the examples.


.. currentmodule:: sklearn

Expand Down