Convert various functions to use keyword-only arguments

[asmeurer]

References to other Issues or PRs

#17944 (shouldn't mark as closed because this PR isn't going to cover every function in SymPy)

Brief description of what is fixed or changed

I am fixing various function signatures to use keyword only arguments. So for example,

def expand(self, deep=True, modulus=None, power_base=True, power_exp=True,
           mul=True, log=True, multinomial=True, basic=True, **hints):

becomes

def expand(self, *, deep=True, modulus=None, power_base=True, power_exp=True,
           mul=True, log=True, multinomial=True, basic=True, **hints):

This means that things like expr.expand(True) are no longer valid. You will have to use expr.expand(deep=True) instead.

Other comments

This is a huge undertaking and I probably won't get to every function in SymPy. Please feel free to merge this at any time if the tests are passing, and to pick up this work.

Release Notes

• core
  • Change several function signatures to use keyword-only arguments.
• functions
  • Change several function signatures to use keyword-only arguments.

[sympy-bot]

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• core

  • Change several function signatures to use keyword-only arguments. (#21803 by @asmeurer)

• functions

  • Change several function signatures to use keyword-only arguments. (#21803 by @asmeurer)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.9.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->

https://github.com/sympy/sympy/issues/17944 (shouldn't mark as closed because this PR isn't going to cover every function in SymPy)

#### Brief description of what is fixed or changed

I am fixing various function signatures to use keyword only arguments. So for example,

```py
def expand(self, deep=True, modulus=None, power_base=True, power_exp=True,
           mul=True, log=True, multinomial=True, basic=True, **hints):
```

becomes

```py
def expand(self, *, deep=True, modulus=None, power_base=True, power_exp=True,
           mul=True, log=True, multinomial=True, basic=True, **hints):
```

This means that things like `expr.expand(True)` are no longer valid. You will have to use `expr.expand(deep=True)` instead.

#### Other comments

This is a huge undertaking and I probably won't get to every function in SymPy. Please feel free to merge this at any time if the tests are passing, and to pick up this work. 

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
- core
  - Change several function signatures to use keyword-only arguments.
- functions
  - Change several function signatures to use keyword-only arguments.
<!-- END RELEASE NOTES -->

[asmeurer]

I should note that this isn't always a trivial fix. In some cases, we really do want to allow keyword arguments to be passed as positional arguments. I'm trying to be conservative here. For instance, I have changed def matches(self, expr, repl_dict={}, old=False) to def matches(self, expr, repl_dict={}, *, old=False) because it seems reasonable to me to pass the repl_dict as a positional argument.

[github-actions]

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

       before           after         ratio
     [78380bf1]       [14dd8937]
!        544±10ns           failed      n/a  core.expand.TimeExpand.time_expand_nothing_todo

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [ed9a550f]       [78380bf1]
     <sympy-1.8^0>                 
+        84.7±6μs       3.68±0.1ms    43.45  matrices.TimeDiagonalEigenvals.time_eigenvals
-        99.3±5μs         64.7±3μs     0.65  matrices.TimeMatrixGetItem.time_ImmutableSparseMatrix_getitem
-        97.3±2μs         61.6±3μs     0.63  matrices.TimeMatrixGetItem.time_MutableDenseMatrix_getitem
-         100±5μs         62.6±3μs     0.62  matrices.TimeMatrixGetItem.time_MutableSparseMatrix_getitem
+      12.3±0.5μs       19.4±0.9μs     1.57  solve.TimeMatrixArithmetic.time_dense_add(3, 0)
+      15.5±0.8μs         34.3±1μs     2.21  solve.TimeMatrixArithmetic.time_dense_add(3, 5)
+      21.8±0.3μs         39.6±2μs     1.82  solve.TimeMatrixArithmetic.time_dense_add(4, 5)
+        38.8±3μs         64.0±3μs     1.65  solve.TimeMatrixArithmetic.time_dense_add(6, 5)
-     1.44±0.09ms         233±10μs     0.16  solve.TimeMatrixArithmetic.time_dense_multiply(10, 0)
-        53.3±2μs         26.5±1μs     0.50  solve.TimeMatrixArithmetic.time_dense_multiply(3, 0)
+        82.5±7μs          152±6μs     1.84  solve.TimeMatrixArithmetic.time_dense_multiply(3, 5)
-         110±3μs       30.8±0.5μs     0.28  solve.TimeMatrixArithmetic.time_dense_multiply(4, 0)
-         318±7μs         64.9±4μs     0.20  solve.TimeMatrixArithmetic.time_dense_multiply(6, 0)
-         106±4μs         47.7±5μs     0.45  solve.TimeMatrixOperations.time_det(3, 0)
-         206±7μs          104±5μs     0.51  solve.TimeMatrixOperations.time_det(3, 2)
-         209±7μs         102±10μs     0.49  solve.TimeMatrixOperations.time_det(3, 5)
-         102±6μs         43.7±3μs     0.43  solve.TimeMatrixOperations.time_det_bareiss(3, 0)
-        199±10μs          119±6μs     0.60  solve.TimeMatrixOperations.time_det_bareiss(3, 2)
-         192±8μs          103±1μs     0.54  solve.TimeMatrixOperations.time_det_bareiss(3, 5)
-         113±3μs         42.7±3μs     0.38  solve.TimeMatrixOperations.time_det_berkowitz(3, 0)
-        202±10μs          105±6μs     0.52  solve.TimeMatrixOperations.time_det_berkowitz(3, 2)
-         194±7μs          104±9μs     0.54  solve.TimeMatrixOperations.time_det_berkowitz(3, 5)
+        842±60μs      1.36±0.04ms     1.62  solve.TimeMatrixOperations.time_det_berkowitz(4, 0)
+     1.03±0.07ms      1.78±0.04ms     1.72  solve.TimeMatrixOperations.time_det_berkowitz(4, 2)
+     1.07±0.04ms      1.95±0.04ms     1.82  solve.TimeMatrixOperations.time_det_berkowitz(4, 5)
+        315±20μs         518±40μs     1.65  solve.TimeMatrixOperations.time_rank(3, 0)
-      5.47±0.4ms       2.95±0.2ms     0.54  solve.TimeRationalSystem.time_linsolve(10)
+      6.64±0.5ms       9.97±0.4ms     1.50  solve.TimeSparseSystem.time_linear_eq_to_matrix(30)
-     1.57±0.02ms         849±50μs     0.54  solve.TimeSparseSystem.time_linsolve_eqs(10)
-     2.68±0.06ms       1.71±0.2ms     0.64  solve.TimeSparseSystem.time_linsolve_eqs(20)
-      3.81±0.1ms       2.36±0.2ms     0.62  solve.TimeSparseSystem.time_linsolve_eqs(30)

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

[oscarbenjamin]

Significantly changed benchmark results (PR vs master)

       before           after         ratio
     [78380bf1]       [14dd8937]
!        544±10ns           failed      n/a  core.expand.TimeExpand.time_expand_nothing_todo

Looks like this causes a benchmark to fail:
https://github.com/sympy/sympy_benchmarks/blob/c9e500f8c7a2c9ed6496103dc7c1278e8a900635/benchmarks/core/expand.py#L10-L20
(I guess bench_expand_32 isn't even run!)

Those benchmarks look broken to me but it makes me wonder if a deprecation warning should be given for this change.

[asmeurer]

But the code in the benchmarks is nonsense. It's calling p.expand(self), which amounts to p.expand(deep=self) where self is the benchmarking class. This is exactly the sort of bug that keyword-only arguments are designed to fix. If there is some case where an argument is used positionally "legitimately", we might consider that a sign that it shouldn't be keyword-only.

[oscarbenjamin]

But the code in the benchmarks is nonsense.

I agree but the thing is that currently that code "works" and the change here breaks it. Also in principle someone could be using this legitimately like p.expand(False). I'm just wondering whether or not there should be a warning rather than causing downstream code to blow up e.g.:

def expand(*args, deep=True, ...):
    if args:
        warn("positional arguments to expand are ignored and will raise an exception soon")
    ...

If we do intend to switch to introduce keyword-only arguments wholesale then there is a lot of potential for breakage, especially with commonly used core routines like expand. A deprecation warning could reference a deprecation removal issue that explains why these are deprecated.

[asmeurer]

If we do do that we should do it with a decorator. It's a little more nontrivial than that. We'd want the existing syntax to continue to work (with a warning), and for help(f) to still show the correct signature (I think it can be done by setting __signature__). I still personally think it's not necessary but I don't feel strongly about it.

[oscarbenjamin]

I'm not sure that it's necessary either but a decorator would be the best approach if it does not lead to significant slowdowns.

@bjodah does your thumbs up above mean that you think we should preserve compatibility with a warning?

Does anyone else have any opinions about this?

[bjodah]

Yes, I think that using deprecation notifications for 1 or 2 releases for all potentially breaking changes is good practice. Using a decorator to achieve this, in this case, sounds like a rather clean way. If there's some (small) performance degradation I think it's acceptable for the transition period as well (since those would soon be removed either way).

[oscarbenjamin]

I'm not sure if 1 or 2 releases is the right way to define it given that we don't have a regular release cycle. Maybe 2 years is a better way to think about it?

It's not uncommon to see bug reports of regressions since versions that are more than 2 years old which shows how often some users update their SymPy installation.

[bjodah]

Sure, time matters more.

[asmeurer]

Our deprecation policy needs to be refined so that we actually specify how long deprecations last. I don't know what the right answer is here, but if we do do a deprecation, the actual length of time isn't so important to decide now. We can remove it when we remove it.