The Wayback Machine - https://web.archive.org/web/20210818174629/https://github.com/scikit-learn/scikit-learn/pull/18293
Skip to content

/ scikit-learn

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.

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

CLN Deprecations position args in PartialDependenceDisplay.plot #18293

Merged
merged 11 commits into from Nov 27, 2020
Merged

CLN Deprecations position args in PartialDependenceDisplay.plot #18293

merged 11 commits into from Nov 27, 2020

Conversation

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Aug 29, 2020

This deprecation warning was left out, while the other plot methods had them.
@thomasjpfan
CLN Deprecations position args in PartialDependenceDisplay.plot
b931eca
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Sep 2, 2020

Isn't it until 0.25?
@alfaro96
alfaro96 approved these changes Sep 2, 2020
View changes
Copy link
Member

@alfaro96 alfaro96 left a comment

Good catch @thomasjpfan!

IIRC @glemaitre, the positional arguments are deprecated from 0.23 and will be removed in 0.25.
@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Sep 2, 2020

_deprecate_positional_args is set to 0.25 by default. Since we have not released this in 0.23, we can:

  1. Release 0.23.3 with this "bug fix"
  2. Include in 0.24 and remove in 0.26.
  3. Include in 0.24 and still remove in 0.25.
@amueller
Copy link
Member

@amueller amueller commented Sep 23, 2020

I vote 2 I think? that seems most consistent and doesn't cost us much?
thomasjpfan added 2 commits Sep 23, 2020
@thomasjpfan
Merge remote-tracking branch 'upstream/master' into deprecate_positio…
6715c4f 
…n_args_plot
@thomasjpfan
ENH Adds version to _deprecate_positional_args
bcb0486
@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Sep 23, 2020

I updated the PR to add a new keyword argument to _deprecate_positional_args so we can pass 0.26 to it.
@amueller
amueller reviewed Nov 20, 2020
View changes
sklearn/utils/validation.py Outdated
The version when positional arguments will result in error. If
callable, then "0.25" will be used for error message.
"""
if callable(version):

This comment has been minimized.

Sign in to view
@amueller

amueller Nov 20, 2020
Member

This is ugly, can't we make version keyword only to fix this, or something?

This comment has been minimized.

Sign in to view
@thomasjpfan

thomasjpfan Nov 23, 2020
Author Member

This trick is used because of how _deprecate_positional_args's first parameter can be string:

@_deprecate_positional_args(version="0.26")
def f():
	pass

or a function:

@_deprecate_positional_args
def g():
	pass

The "nicer" solution would be to always provide the version, but that would be a much bigger diff.

As a side note, this trick is used in Python's lru_cache:

https://github.com/python/cpython/blob/63298930fb531ba2bb4f23bc3b915dbf1e17e9e1/Lib/functools.py#L479

@amueller
Copy link
Member

@amueller amueller commented Nov 20, 2020

marked this as a blocker as we really should include this in the release as we're already behind.
@cmarmo cmarmo added this to the 0.24 milestone Nov 24, 2020
@cmarmo
Copy link
Member

@cmarmo cmarmo commented Nov 25, 2020

Hi @thomasjpfan sorry for doing the bad cop... Do you mind solving conflicts here, I think this PR is quite good to go, right?
@jeremiedbb
Copy link
Member

@jeremiedbb jeremiedbb commented Nov 25, 2020

@thomasjpfan I found a solution here https://stackoverflow.com/questions/3888158/making-decorators-with-optional-arguments/24617244#24617244 that might be more intuitive and make the code a bit cleaner. The structure is the following:

def _deprecate_positional_args(func=None, *, version="0.25"):
    def _decorate(f):
        # here goes all the previous code of _deprecate_positional_args

    if func is not None:
        return _decorate(func)

    return _decorate
thomasjpfan added 2 commits Nov 25, 2020
@thomasjpfan
Merge remote-tracking branch 'upstream/master' into deprecate_positio…
2c880dc 
…n_args_plot
@thomasjpfan
CLN Uses suggestion
709df19
@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Nov 25, 2020

Updated PR with @jeremiedbb's suggestion.
@jeremiedbb
jeremiedbb reviewed Nov 26, 2020
View changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Thanks @thomasjpfan. The docstring does not reflect the new implementation, I made some comments.
sklearn/utils/validation.py Outdated Show resolved Hide resolved
sklearn/utils/validation.py Outdated Show resolved Hide resolved
sklearn/utils/validation.py Outdated Show resolved Hide resolved
sklearn/utils/validation.py Outdated Show resolved Hide resolved
@glemaitre
glemaitre reviewed Nov 26, 2020
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment

In addition to @jeremiedbb suggestions, I miss a test in test_partial_dependence to be sure that we raise the warning. Besides, we need a what's new entry.
thomasjpfan added 3 commits Nov 26, 2020
@thomasjpfan
Merge remote-tracking branch 'upstream/master' into deprecate_positio…
c64a710 
…n_args_plot
@thomasjpfan
DOC Update
cd6cd41
@thomasjpfan
DOC Adds whats new
95dfab5
@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Nov 26, 2020

I miss a test in test_partial_dependence to be sure that we raise the warning.

We avoided doing when working on _deprecate_positional_args because there would have been too many tests. In these cases, we are trusting that the test for _deprecate_positional_args works.
@thomasjpfan
DOC Use meth
3068fc6
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Nov 26, 2020

OK, works with me then :)
@glemaitre
glemaitre approved these changes Nov 26, 2020
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment

LGTM
thomasjpfan added 2 commits Nov 26, 2020
@thomasjpfan
Merge remote-tracking branch 'upstream/master' into deprecate_positio…
01a390b 
…n_args_plot
@thomasjpfan
REV Less diffs
fd5a257
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Nov 27, 2020

@jeremiedbb Do you want to make another pass?
@jeremiedbb
jeremiedbb approved these changes Nov 27, 2020
View changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

lgtm
@jeremiedbb jeremiedbb merged commit f82525e into scikit-learn:master Nov 27, 2020
26 checks passed
26 checks passed
@github-actions
triage
Details
@github-actions
Check build trigger
Details
@github-actions
Build wheel for cp${{ matrix.python }}-${{ matrix.platform_id }}
Details
@github-actions
Source distribution
Details
@github-actions
Upload to Anaconda
Details
@lgtm-com
LGTM analysis: C/C++ No code changes detected
Details
@lgtm-com
LGTM analysis: JavaScript No code changes detected
Details
@lgtm-com
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
@circleci-artifacts-redirector
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
@codecov
codecov/patch 100.00% of diff hit (target 98.18%)
Details
@codecov
codecov/project 98.18% (+0.00%) compared to eaa45c8
Details
@azure-pipelines
scikit-learn.scikit-learn Build #20201126.43 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linting) Linting succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py36_conda_openblas) Linux py36_conda_openblas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py36_ubuntu_atlas) Linux py36_ubuntu_atlas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux32 py36_ubuntu_atlas_32bit) Linux32 py36_ubuntu_atlas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux_Runs pylatest_conda_mkl) Linux_Runs pylatest_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py36_pip_openblas_32bit) Windows py36_pip_openblas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_forge_mkl) macOS pylatest_conda_forge_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_mkl_no_openmp) macOS pylatest_conda_mkl_no_openmp succeeded
Details
@jeremiedbb
Copy link
Member

@jeremiedbb jeremiedbb commented Nov 27, 2020

thanks @thomasjpfan !
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amueller amueller

@glemaitre glemaitre

@alfaro96 alfaro96

@jeremiedbb jeremiedbb

Assignees
No one assigned
Labels
Blocker module:inspection
Projects
None yet
Milestone
0.24
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@thomasjpfan @glemaitre @amueller @cmarmo @jeremiedbb @alfaro96