DOC: Clarify argument documentation for `solve` #14402

Kaniee · 2021-07-13T11:51:46Z

What does this implement/fix?

There was a minor styling error for the transpose keyword.
While fixing this, I took the opportunity to make the documentation on the other keywords a bit clearer and added some lines to document the default values.

Additional information

Since this is my first PR to scipy I highly appreciate your feedback on the Docstring/Commit-Messages/PR/...

mdhaber

This is a helpful first PR. I agree that this is an improvement.

What exactly was the styling error on the transposed keyword?

Commit messages are good. After e.g. "DOC" and "STY", I've been told to add the subpackage (DOC: linalg: ..., STY: linalg: ), but not many people seem to do that consistently.

The messages could be a little more specific - after looking at the PR, I can't tell what the styling issue was for the transposed keyword, and the second commit adds default values for parameters of solve. (But they are already better than most.)

Also, take a look at what the numpydoc style guide has to say about default values of parameters. What you have is fine, but instead of:

transposed : bool, optional
...
Default is False.

you are also allowed to write:

transposed : bool, default: False

Up to you. Just wanted you to know that it was an option.

mdhaber · 2021-07-15T17:00:54Z

scipy/linalg/basic.py

@@ -41,8 +41,8 @@ def solve(a, b, sym_pos=False, lower=False, overwrite_a=False,
          overwrite_b=False, debug=None, check_finite=True, assume_a='gen',
          transposed=False):
    """
-    Solves the linear equation set ``a * x = b`` for the unknown ``x``
-    for square ``a`` matrix.
+    Solves the linear equation set ``ax = b`` for the unknown ``x`` for


Suggested change

Solves the linear equation set ``ax = b`` for the unknown ``x`` for

Solves the linear equation set ``a@x = b`` for the unknown ``x`` for

I agree that * was not the right operator, but if this is going to be written as code (between double backticks), consider using the closest operator. I understand that this isn't perfect (we're not using = as an assignment operator here) but it's closer.

How about changing it to single backticks and write a x = b or ax = b (as in numpy.linalg.solve)?
Personally, I'd prefer to use a more mathematical syntax in the documentation over Python syntax.
But I'm also fine with a @ x = b, if this is preferred way of expressing mathematical equations in the scipy docs.

mdhaber · 2021-07-15T17:03:33Z

scipy/linalg/basic.py

+        If True, the calulation is based only on the data in the lower triangle of
+        matrix `a` instead of data in the upper triangle.


Suggested change

If True, the calulation is based only on the data in the lower triangle of

matrix `a` instead of data in the upper triangle.

If True, the calculation uses only the data in the lower triangle of

matrix `a`; entries above the diagonal are ignored.

mdhaber · 2021-07-15T17:04:27Z

scipy/linalg/basic.py

-        for complex matrices (only for True).
+        Default is ``'gen'``.
+    transposed : bool, optional
+        If True, calculate ``a^T x = b``.


Consider:

Suggested change

If True, calculate ``a^T x = b``.

If True, solve ``a.T @ x = b``.

I would leave something in there about raising an error for complex matrices when it's True.

This is something I'm planning to solve with the new version of solve. Then it would be possible to do it with reals and doubles hopefully with a better API.

mdhaber · 2021-07-15T17:09:52Z

scipy/linalg/basic.py

+        If True, the calulation is based only on the data in the lower triangle of
+        matrix `a` instead of data in the upper triangle.
+        Ignored if ``assume_a == 'gen'``.
+        Default is False.


It is implicit, but consider writing out what this means: the calculation uses only data in the upper triangle; entries below the diagonal are ignored.

mdhaber · 2021-07-15T19:14:19Z

scipy/linalg/basic.py

    lower : bool, optional
-        If True, only the data contained in the lower triangle of `a`. Default
-        is to use upper triangle. (ignored for ``'gen'``)
+        If True, the calulation is based only on the data in the lower triangle of


This line is a few characters too long; it's causing the failure in "scipy.scipy (Main Lint)"

The other failures seem to be in master right now.

True, I didn't see that. I'll fix it

ilayn · 2021-07-15T19:23:09Z

The whole transposed keyword, obviously in hindsight, was a mistake. We should have thought it a bit better. Other than that this looks good to me.

mdhaber · 2022-09-28T07:55:33Z

@Kaniee would you like to finish this up? If you need help resolving merge conflicts, please let me know. Update: I went ahead and merged main.

scipy/linalg/_basic.py

mdhaber

I think this is an improvement. It makes the defaults more obvious, corrects some backticks, and clarifies the description of lower.
@j-bowhay looks like you came by here. What do you think?

j-bowhay

@mdhaber two minor comments from me, neither are blocking.

scipy/linalg/_basic.py

j-bowhay · 2022-10-07T08:03:40Z

scipy/linalg/_basic.py

        Whether to check that the input matrices contain only finite numbers.
        Disabling may give a performance gain, but may result in problems
        (crashes, non-termination) if the inputs do contain infinities or NaNs.
-    assume_a : str, optional
+    assume_a : str, {'gen', 'sym', 'her', 'pos'}


Suggested change

assume_a : str, {'gen', 'sym', 'her', 'pos'}

assume_a : str, {'gen', 'sym', 'her', 'pos'}, default: 'gen'

Given all the other arguments state the default it would be nice to do the same here for consistency

From the style guide

When a parameter can only assume one of a fixed set of values, those values can be listed in braces, with the default appearing first.

The example does not also call out the default explicitly, so this follows the example.

Thanks didn't realise this was the case; you learn something new everyday!

[skip ci] Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>

mdhaber · 2022-10-07T09:07:03Z

Thanks @j-bowhay. Comments addressed.

@Kaniee I'll go ahead and merge this, but if you'd like to propose that equations look more like math than code throughout linalg, I think that sounds helpful. Here I suggested maintaining the existing style (but clarifying) because that was recommended to me a long time ago in the context of optimize.linprog IIRC, but it is somewhat inconsistent throughout linalg. There is one example of an italicized equation like you suggest, and there is this one that is teletype, and the rest are in plain text! Picking a standard (other than plain text) would be preferable, I think!

DOC: linalg.solve: clarify documentation Co-authored-by: Matt Haberland <mhaberla@calpoly.edu> Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>

Kaniee added 2 commits July 13, 2021 12:38

STY: Fix styling for transposed keyword in solve

d74f210

DOC: Add default values for solve

90b33bd

Kaniee requested review from ilayn and larsoner as code owners July 13, 2021 11:51

mdhaber reviewed Jul 15, 2021

View reviewed changes

rgommers added Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org scipy.linalg labels Jul 20, 2021

Merge remote-tracking branch 'upstream/main' into doc-solve

7b398c8

j-bowhay added the needs-work Items that are pending response from the author label Oct 3, 2022

mdhaber reviewed Oct 7, 2022

View reviewed changes

scipy/linalg/_basic.py Outdated Show resolved Hide resolved

scipy/linalg/_basic.py Outdated Show resolved Hide resolved

Apply suggestions from code review

9def684

mdhaber approved these changes Oct 7, 2022

View reviewed changes

j-bowhay reviewed Oct 7, 2022

View reviewed changes

Update scipy/linalg/_basic.py

8bbfc09

[skip ci] Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>

mdhaber merged commit acac275 into scipy:main Oct 7, 2022

mdhaber added this to the 1.10.0 milestone Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Clarify argument documentation for `solve` #14402

DOC: Clarify argument documentation for `solve` #14402

Kaniee commented Jul 13, 2021

mdhaber left a comment •

edited

mdhaber Jul 15, 2021

Kaniee Jul 16, 2021

mdhaber Jul 15, 2021

mdhaber Jul 15, 2021

mdhaber Jul 15, 2021

ilayn Jul 15, 2021

mdhaber Jul 15, 2021

mdhaber Jul 15, 2021 •

edited

Kaniee Jul 16, 2021

ilayn commented Jul 15, 2021

mdhaber commented Sep 28, 2022 •

edited

mdhaber left a comment

j-bowhay left a comment

j-bowhay Oct 7, 2022

mdhaber Oct 7, 2022

j-bowhay Oct 7, 2022

mdhaber commented Oct 7, 2022 •

edited

	Solves the linear equation set ``ax = b`` for the unknown ``x`` for
	Solves the linear equation set ``a@x = b`` for the unknown ``x`` for

		If True, the calulation is based only on the data in the lower triangle of
		matrix `a` instead of data in the upper triangle.

	If True, calculate ``a^T x = b``.
	If True, solve ``a.T @ x = b``.

	assume_a : str, {'gen', 'sym', 'her', 'pos'}
	assume_a : str, {'gen', 'sym', 'her', 'pos'}, default: 'gen'

DOC: Clarify argument documentation for solve #14402

DOC: Clarify argument documentation for solve #14402

Conversation

Kaniee commented Jul 13, 2021

What does this implement/fix?

Additional information

mdhaber left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhaber Jul 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilayn commented Jul 15, 2021

mdhaber commented Sep 28, 2022 • edited

mdhaber left a comment

Choose a reason for hiding this comment

j-bowhay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdhaber commented Oct 7, 2022 • edited

DOC: Clarify argument documentation for `solve` #14402

DOC: Clarify argument documentation for `solve` #14402

mdhaber left a comment •

edited

mdhaber Jul 15, 2021 •

edited

mdhaber commented Sep 28, 2022 •

edited

mdhaber commented Oct 7, 2022 •

edited