Functions should list explicit keyword arguments when possible

[asmeurer]

Whenever possible, functions should list the explicit keyword arguments in their signature, rather than using **kwargs. Furthermore, if **kwargs is not passed through to anything, it should not be used. This makes the functions more self documenting to things like help() and ? in IPython that show the signature, and it helps avoid silent typo errors.

This is an example of what I'm talking about.

I noticed this for instance in legendre_poly, which has **args but only uses the polys flag, but I know there are plenty of other instances all over the codebase.

[asmeurer]

Oh apparently legendre_poly was already fixed. But we should review the code for other instances of this. Since this is the second example I've seen of this in as many weeks, I know there must be many other instances of this problem.

[asmeurer]

In some instances this won't be possible, because keyword arguments after *args is not allowed in Python 2. For those cases, we should make sure the keyword arguments are documented well, and add a comment to change them to explicit keyword arguments when Python 2 support is dropped.

[vivonk]

In some instances this won't be possible, because keyword arguments after *args is not allowed in Python 2

+1, some reference can be seen here

but in python 3 keyword arguments can be used after *args. But I think the modules are same for python 2 & 3, is it?
I would like to work on this so @asmeurer can you tell about above question

[asmeurer]

Yes, all code in SymPy must work in both Python 2 and Python 3 for now.

[abaokar-07]

What functions do we need to change?

[vivonk]

@asmeurer should I try in replace in whole modules or in some modules?

[normalhuman]

@vivonk One module at a time may be a good approach. Easier to review. (E.g., physics module, or geometry module, or solvers module, etc)

[vivonk]

if **kwargs is not passed through to anything, it should not be used

Is it mean that if the **kwargs is passed but not used in that particular function then it should be removed from there ? for example here
@normalhuman @asmeurer

[normalhuman]

In that example, **kwargs indeed looks like a typo. It's not used by the function, and nobody is trying to pass such keyword arguments to that function, either. Yes, should be removed.

If something else was indeed passing keyword arguments to the function, then **kwargs should not be just removed; that would raise "unexpected keyword argument" error. In such a situation one has to consider why something is passing an argument that the function ignores.

[gxyd]

Is this issue #13683 to similar to this?

[vivonk]

@normalhuman @asmeurer I am facing a problem. In some cases it is like

1. In first function **kwargs is passed and then the **kwargs value is used in calling another function. To write down the explicit arguments for the first function I should know the kwargs of the another function too. Should I check arguments of the another function and then change it accordingly or something else?
2. Another is there are three arguments which are wrapped in **kwargs so what should be the preference of those keyword arguments in the case of removing **kwargs
   for example, this is the function def classify_ode(eq, func=None, dict=False, ics=None, **kwargs):
   suppose I am removing **kwargs where the explicit keyword arguments are prep, n, x0.
   So after removing **kwargs I am thinking to put in this manner
   def classify_ode(eq, func=None, dict=False, ics=None, prep = True, x0 = 0, n = 100):, what should be the sense of ordering the keyword arguments?

[asmeurer]

@gxyd yes that issue is related. In the case of #13683, there are functions that use **kwargs for which it cannot be made more explicit, because the options flags used in many functions and passed around (the polys work this way, for the most part). In that case, we should just make sure that all the options are documented well so that you can easily tell what options a function can take.

[asmeurer]

In first function **kwargs is passed and then the **kwargs value is used in calling another function. To write down the explicit arguments for the first function I should know the kwargs of the another function too. Should I check arguments of the another function and then change it accordingly or something else?

This is what I meant by "passed through". In those cases, it is generally best to leave the **kwargs for the options that are passed through (and make sure it is documented). Any option that isn't passed through (like kwargs.pop(option)) should be documented.

Another is there are three arguments which are wrapped in **kwargs so what should be the preference of those keyword arguments in the case of removing **kwargs

All new keyword arguments should be put after the existing ones. Once we drop Python 2 support, we can make these keyword arguments keyword-only so that the order doesn't matter. For now, we have to use some order. In the case of something like classify_ode, only func should be considered positional. Relying on the order for any of the other flags should be discouraged, because when we drop Python 2 support way may change the signature to classify_ode(eq, func=None, *, dict=False, ics=None, prep=True, x0=0, n=100).

Also, don't put spaces around = when using them in a function signature (see PEP 8).

[vivonk]

@asmeurer got your point. Thanks to clear it

[ylemkimon]

I think in cases passing through kwargs, we should explicitly pass only needed arguments or use a custom options class to prevent unwanted side effects and user errors.

[asmeurer]

I agree such options refactoring would be nice. However it's a lot more refactoring than the basic change I am talking about here (definitely not "easy to fix").

[rajpratyush]

is this still needs work?

[Jay-Patankar]

@asmeurer @friyaz
I am a beginner and feel like I have understood this issue . I need to scan functions in modules and replace **kwargs with explicit keyword parameters only if the following conditions hold true:
1] There is no *args in the signature
2] Very less keyword parameters are passed hence no need of **kwargs

Please confirm if I have understood it right.
Also it seems from the above discussion that some modules are already refactored ......so can you point out modules which are yet to be done....so that I can start working on them.

[asmeurer]

The use of *args is irrelevant. The only criterion is that there are explicit known keyword arguments that are used in the function. You'll generally see something like kwargs.get('argname', default_value) in the function body. If that's the case, it should be in the signature as argname=default_value instead. If **kwargs are passed through to other functions, then **kwargs should be left intact alongside them, as that means arbitrary keyword arguments are accepted. Otherwise, if only specific keyword arguments are accepted, the **kwargs should be removed.

[Jay-Patankar]

Thanks for replying @asmeurer . Before I proceed can you just confirm whether my perception is correct with the help of following examples:

def _eval_evalf(self, prec=15, **options):
        f, (t, a, b) = self.args
        dps = prec_to_dps(prec)
        f = tuple([i.evalf(n=dps, **options) for i in f])
        a, b = [i.evalf(n=dps, **options) for i in (a, b)]
        return self.func(f, (t, a, b))

Here options is passed to another function inside body , so nothing needs to be done. (Keep **options as it is).

def __new__(cls, *args, **kwargs):
        # Points are sequences, but they should not
        # be converted to Tuples, so use this detection function instead.
        def is_seq_and_not_point(a):
            # we cannot use isinstance(a, Point) since we cannot import Point
            if hasattr(a, 'is_Point') and a.is_Point:
                return False
            return is_sequence(a)

Here **kwargs can be removed since it is not used in the body at all.

def __new__(cls, *args, **kwargs):
        evaluate = kwargs.get('evaluate', global_parameters.evaluate)

         flatten inputs to merge intersections and iterables
        args = _sympify(args)

        # Reduce sets using known rules
        if evaluate:
            args = list(cls._new_args_filter(args))
            return simplify_union(args)

        args = list(ordered(args, Set._infimum_key))

        obj = Basic.__new__(cls, *args)
        obj._argset = frozenset(args)
        return obj

Here **kwargs should be replaced with (evaluate=global_parameters.evaluate)

[sylee957]

I had left some comment in #25752 that the correct way to migrate the existing code after PEP-3102

def foo(**kwargs):
   kwargs.get('key1')
   kwargs.get('key2')
   kwargs.get('key3', default)

is not using

def foo(key1=val1, key2=val2, key3=default):
   ...

but using

def foo(*, key1, key2, key3=default):
   ...

should be the correct and isomorphic way to migrate code without breaking any backward compatibiity,
and also without introducing any new careless feature like being able to sue key1, key2 as positional arguments.

• You shouldn't just turn the keyword arguments into both keyword-and-positional arguments, but keyword only arguments.
  Or it is possible to use keyword arguments like foo(1, 2, 3).
  Positional arguments are useful, for example, mathematical functions,
  but is not useful, and it does not make sense, for example, if you are doing something like simplify options or plot options.

• You should only add default argument if the original code was kwargs.get(..., None) If the original code didn't have default value, you shouldn't add new hypothetical default value (like None) unless it is intended.

I'm just adding this guide because it is not very easy to find this info, unless you can consult some other advices from experienced programmers in Python, because it is not written in most of the tutorials about using niche features like PEP-3102

[asmeurer]

@sylee957 the default value of the second argument to dict.get is None.

>>> print({}.get('x'))
None

So changing kwargs.get(key) to key instead of key=None would be a backwards compatibility break, and a fairly major one at that for a public function since it adds a new required argument so would break any existing calls to it.

Required keyword arguments should be a fairly rare thing to want to include in a signature.

[sylee957]

Required keyword arguments should be a fairly rare thing to want to include in a signature

Why?

[sylee957]

So changing kwargs.get(key) to key instead of key=None

I may got trivially wrong about kwargs.get,
But there are other cases like kwargs[key] was used instead, which can be distinguished.

Also, in the original PR I linked to, default values None doesn't make sense anyway, so for that specific case, I would defend my argument

[sylee957]

Anyway, I think that the major problem is having poorly defined API based on kwargs, bad ergonomics,

I don't think that I would agree that sympy should have indefinite API that relies on **kwargs.

This can misguide people that they can extend the features indefinitely, which follows up with a poor effort to document about the API.

I haven't counted on, but I have seen fairly many sympy functions to have **kwargs without what is about, or partially documented.

[oscarbenjamin]

I think that having this as an "easy to fix" issue is not helpful. It could be considered suitable for a new contributor if there was a clear list of which functions to change but there are lots of functions that take **kwargs and most of them should not be changed. Needing to judge what things to change makes this not really suitable for new contributors.

[sylee957]

I agree that we should remove easy-to-fix.
But I think that it is just a shame that we need some "decision" to figure out and change the semantics of kwargs, which could just be done with some toolings or language support.

[Jay-Patankar]

@sylee957 @asmeurer @oscarbenjamin
I found this discussion thread quite useful as a beginner . Agreed that this issue is not that beginner friendly but since I had consulted with you for my commits in PR #25752 , requesting you to merge my PR . Also I am looking for some good issues to solve as a part of the hacktoberfest .....please suggest me similar issues .