DEP: adjust deprecation of positional arguments #18624

j-bowhay · 2023-06-04T10:08:27Z

Reference issue

What does this implement/fix?

In the past, when deprecating an argument to a function that we want to remove, we have used the argument being different from the default value of None to trigger the deprecation warning. However, as pointed out by @rgommers, the current method we are using to deprecate arguments we want to remove was not entirely watertight if the user uses all positional arguments.

As an example, kendalltau(x, y, None, 'omit', 'exact') would not raise the deprecation warning so we are in danger of breaking the user's code without warning if this was removed directly. Side note, this is yet another reason why making new code use keyword-only arguments when applicable is desirable.

As per @rgommers suggestion, I have added a private _NoValue object which is used to trigger the deprecation warning. This means now:

In [2]: kendalltau([1,4], [1,2], None, 'omit', 'exact')
<ipython-input-2-e55511017c59>:1: DeprecationWarning: 'kendalltau' keyword argument 'initial_lexsort' is deprecated as it is unused and will be removed in SciPy 1.12.0.
  kendalltau([1,4], [1,2], None, 'omit', 'exact')

as required to remove the argument.

Additional information

cc @h-vetinari

h-vetinari

If we're extending the deprecation by two releases, I'd like to fully deprecate positional use of all kwargs along the lines of #14714. Example below.

h-vetinari · 2023-06-06T23:18:10Z

scipy/integrate/_quadrature.py

@@ -531,7 +532,7 @@ def simps(y, x=None, dx=1.0, axis=-1, even=None):
    return simpson(y, x=x, dx=dx, axis=axis, even=even)


-def simpson(y, x=None, dx=1.0, axis=-1, even=None):
+def simpson(y, x=None, dx=1.0, axis=-1, even=_NoValue):


This is how things would look like after switching to keyword-only args.

Suggested change

def simpson(y, x=None, dx=1.0, axis=-1, even=_NoValue):

def simpson(y, *, x=None, dx=1.0, axis=-1):

In the meantime, we should probably vendor the helper that scikit-learn came up with to do the deprecation of positional use.

I am +1000 there!

h-vetinari · 2023-06-06T23:23:01Z

scipy/integrate/_quadrature.py

@@ -522,7 +523,7 @@ def _basic_simpson(y, start, stop, x, dx, axis):

 # Note: alias kept for backwards compatibility. simps was renamed to simpson
 # because the former is a slur in colloquial English (see gh-12924).
-def simps(y, x=None, dx=1.0, axis=-1, even=None):
+def simps(y, x=None, dx=1.0, axis=-1, even=_NoValue):
    """An alias of `simpson`.

    `simps` is kept for backwards compatibility. For new code, prefer


Sidenote: this alias sounds like it should eventually be removed. Looking at the above-mentioned #12924, it's even more of a candidate for deprecation and removal.

rgommers · 2023-06-11T17:57:29Z

If we're extending the deprecation by two releases, I'd like to fully deprecate positional use of all kwargs along the lines of #14714. Example below.

It seems to me like this is trying to reopen the gh-14714 discussion, and it's not really related to the topic of this PR - which is only about ensuring warnings for already deprecated keywords are always emitted. I'd suggest not mixing new deprecations into this PR.

rgommers · 2023-06-14T19:48:18Z

I couldn't follow the discussion here but could someone let me know what the implication would be for #18488 ?

You're deprecating the tol keyword there, right? You want to be using the same _NoValue pattern as in this PR. Right now, I believe in your branch, something like bicg(A, b, x0, None, 100) won't raise a deprecation warning (because it's gated by tol is not None), and then once tol gets removed that code breaks. The purpose of the _NoValue pattern is to distinguish between "user doesn't use keyword" and "user uses keyword explicitly with its default value"

ilayn · 2023-06-14T21:35:08Z

OK, that means, I'll wait for a deprecation cycle starting from 1.12 to 1.14 if anybody sets tol with positional arg which is your example. Then 1.14 rips tol out. But in the meantime am I also putting * for strict kwarging? Because I want to :) since there are like 4 different deprecations going on simultaneously in the iterative solvers. I got a bit tired of them just by looking.

That was the part I didn't parse in this discussion.

h-vetinari · 2023-06-14T23:46:12Z

But in the meantime am I also putting * for strict kwarging?

That's gonna need an additional deprecation for those people feeding the current kwargs positionally. This is what the above-promised issue will be about, because doing that deprecation is more involved (if starting from scratch, which is why we should build on the work from scikit-learn on this).

So no * just yet, but also for 1.14 once the depreciation expires.

rgommers · 2023-06-15T08:44:06Z

So no * just yet, but also for 1.14 once the depreciation expires.

This is right.

That's gonna need an additional deprecation for those people feeding the current kwargs positionally.

It may not, depending on the details - in @ilayn's case I think this does not require an additional deprecation since tol is already the first keyword of the ones of interest (I suspect leaving x0 as positional is fine/desired, because it's not a meaningful name). Current signature:

bicg(A, b, x0=None, tol=None, maxiter=None, M=None, callback=None, atol=0., rtol=1e-5)

New signature for 1.14.0 with only the deprecation that's already there plus the _NoValue pattern:

# You may also reorder the keywords at that point if you desire to do so for clarity
bicg(A, b, x0=None, *, maxiter=None, M=None, callback=None, atol=0., rtol=1e-5)

There's of course more issues (e.g. M is a very bad name); if you'd have the freedom to start from scratch it'd be more like:

# Order these keywords from most to least used
bicg(A, b, /, *, guess=None, atol=0., rtol=1e-5, maxiter=None, preconditioner=None, callback=None)

... (i.e. "please use keyword arguments").

@h-vetinari perhaps you can follow up with a minor concrete tweak to @ilayn's deprecation message? I suspect it may be annoying to get that right (because you don't know when it applies without more introspection) and it may not be worth it, but please do try to make it concrete, because it'll be obvious once you actually try.

ilayn · 2023-06-15T10:59:57Z

Ah OK it's much clearer for me now too. This PR looks indeed then ready to go, and we can do some follow ups right after this. 1.12 looks like the big clean-up with all the fortran, signatures, and deprecations and so on :) Let me take the discussion to #18488 then and what it would take to mark it as down.

h-vetinari · 2023-06-15T22:07:54Z

@h-vetinari perhaps you can follow up with a minor concrete tweak to @ilayn's deprecation message? I suspect it may be annoying to get that right (because you don't know when it applies without more introspection) and it may not be worth it, but please do try to make it concrete, because it'll be obvious once you actually try.

In the spirit of decoupling the details of the "deprecate positional use" from the current PRs, how about we deal with it more holistically in a separate issue/PR (but definitely for 1.12)? That way, all the existing PRs have to do is use _NoValue instead of None for deprecated kwargs.

rgommers · 2023-06-16T08:24:25Z

Sounds good to me!

ilayn · 2023-06-18T09:23:00Z

@j-bowhay Oops, I created a conflict, sorry about that. In the meantime, I'm preparing another PR for iterative kwargs having _NoObject. Should I wait for you or get the declaration in the new PR? Both are fine, just don't want to step on your toes. If you take out the iterative part I can continue with them after this too in case that seems less work.

j-bowhay · 2023-06-18T17:20:58Z

@ilayn I have hopefully addressed all @rgommers comments so this should be good to go if it comes back green. I ended up removing any of the changes to the iterative solver as I thought it would be easier to add it to your pr and do them all at once rather than hunt through the merge conflicts for my changes

h-vetinari · 2023-06-19T06:35:27Z

This is what the above-promised issue will be about

#18703

ilayn · 2023-06-21T12:06:03Z

Just to confirm, is everyone happy to click the button about this?

ilayn · 2023-06-22T10:54:25Z

All thumbs up from the participants and thanks all for the discussions. It is quite informative for me too.

j-bowhay added 6 commits June 3, 2023 14:36

DEP: signal: update deprecations to allow for argument removal

d97427c

DEP: stats: update deprecation of stats.kendalltau kwarg initial_lexsort

df4359a

DEP: sparse.linalg: update restrt deprecation

b4563dd

DEP: move _NoValue object to a central location

2137644

DEP: special: update deprecation of comb legacy argument

b65dad8

DEP: integrate: update simpson even argument deprecation

58c322c

j-bowhay requested review from person142, larsoner and ilayn as code owners June 4, 2023 10:08

github-actions bot added scipy._lib scipy.integrate scipy.signal scipy.sparse scipy.sparse.linalg scipy.special scipy.stats labels Jun 4, 2023

j-bowhay added maintenance Items related to regular maintenance tasks deprecated Items related to behavior that has been deprecated and removed scipy.stats scipy.special scipy.sparse.linalg scipy.sparse scipy.signal scipy.integrate scipy._lib labels Jun 4, 2023

j-bowhay added this to the 1.12.0 milestone Jun 4, 2023

j-bowhay requested a review from h-vetinari June 6, 2023 15:24

j-bowhay mentioned this pull request Jun 6, 2023

DOC/DEP: update documentation on deprecation process #18638

Draft

h-vetinari reviewed Jun 6, 2023

View reviewed changes

j-bowhay added 3 commits June 18, 2023 18:07

Merge remote-tracking branch 'upstream/main' into dep_adjustments

21d9387

DEP: handle none case in nyq helper

b29af5b

DEP: handle none case in simpson adjustment

a919d82

github-actions bot added scipy._lib scipy.integrate scipy.signal scipy.special scipy.stats labels Jun 18, 2023

j-bowhay mentioned this pull request Jun 18, 2023

DEP: integrate: deprecate old aliases #18699

Merged

ilayn merged commit fcf74db into scipy:main Jun 22, 2023

ilayn mentioned this pull request Jun 22, 2023

MAINT:sparse.linalg: Use _NoValue for deprecated kwargs #18723

Merged

steppi mentioned this pull request Jun 22, 2023

ENH: add stirling2 function to scipy.special #18103

Merged

This was referenced Jul 8, 2023

DEP: vendor sklearn's mechanism to deprecate passing kwargs positionally #18843

Merged

META: deprecation tracker #15765

Open

	def simpson(y, x=None, dx=1.0, axis=-1, even=_NoValue):
	def simpson(y, *, x=None, dx=1.0, axis=-1):

Uh oh!

DEP: adjust deprecation of positional arguments #18624

DEP: adjust deprecation of positional arguments #18624

Uh oh!

Conversation

j-bowhay commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference issue

What does this implement/fix?

Additional information

Uh oh!

h-vetinari left a comment

Choose a reason for hiding this comment

Uh oh!

h-vetinari Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

tupui Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

h-vetinari Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-vetinari Jun 19, 2023

Choose a reason for hiding this comment

Uh oh!

rgommers commented Jun 11, 2023

Uh oh!

rgommers commented Jun 14, 2023

Uh oh!

ilayn commented Jun 14, 2023

Uh oh!

h-vetinari commented Jun 14, 2023

Uh oh!

rgommers commented Jun 15, 2023

Uh oh!

ilayn commented Jun 15, 2023

Uh oh!

h-vetinari commented Jun 15, 2023

Uh oh!

rgommers commented Jun 16, 2023

Uh oh!

ilayn commented Jun 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-bowhay commented Jun 18, 2023

Uh oh!

h-vetinari commented Jun 19, 2023

Uh oh!

ilayn commented Jun 21, 2023

Uh oh!

ilayn commented Jun 22, 2023

Uh oh!

Uh oh!

j-bowhay commented Jun 4, 2023 •

edited

Loading

h-vetinari Jun 6, 2023 •

edited

Loading

ilayn commented Jun 18, 2023 •

edited

Loading