Skip to content
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

DOC: optimize: remove inadvertent block quote indentation #13106

Closed
wants to merge 5 commits into from

Conversation

mdhaber
Copy link
Contributor

@mdhaber mdhaber commented Nov 21, 2020

Many lists in optimize documentation are indented relative to the surrounding text. The indentation indicates to Sphinx that the
text is to be formatted as a block quote, and our theme represents that with a vertical bar. Much of this appears to be unintentional, and it's understandable since we're used to indenting all blocks in Python, but Sphinx has other ideas.

image

vs

image

This commit removes unnecessary indentation to eliminate these vertical bars.

I know we typically don't like these large cleanups, but I thought this might deserve an exception. Contributors (like me) will continue to copy the formatting they see in other documentation, assuming it is supposed to be that way, and rather than whittling down the bad formatting over time, it will continue to grow. So I thought maybe this deserves to be considered. Hopefully, it's pretty uncontroversial as it's almost all whitespace changes.

@@ -142,13 +142,13 @@ def linprog_terse_callback(res):
feasible solution is sought and the T has an additional row
representing an alternate objective function.
status : int
An integer representing the exit status of the optimization::
An integer representing the exit status of the optimization:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another thing that shows up occasionally - rendering return code lists as literal blocks. One could argue that it's supposed to be that way, but I think it's more important for it to be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Literal blocks are easier to align on the screen, otherwise you get wonky lists which look quite ugly.

Copy link
Contributor Author

@mdhaber mdhaber Nov 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Literal blocks are easier to align on the screen, otherwise you get wonky lists which look quite ugly.

I agree that the bullets are too far to the left, but isn't that more of a problem with the Sphinx theme? I think that's something we can (and should) fix across the board with a change in the right place.

This change isn't being displayed correctly.

Thanks. It was supposed to be more like:
image

I'll fix.

Copy link
Contributor Author

@mdhaber mdhaber Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many lists in the optimize documentation are indented relative to the
surrounding text. The indentation indicates to Sphinx that the
text is to be formatted as a block quote, and our theme represents
that with a vertical bar. Much of this appears to be unintentional.
This commit removes the extra indentation to eliminate these
vertical bars.
@andyfaff
Copy link
Contributor

This kind of change isn't going to result in a lot of PRs needing to be rebased to fix merge conflicts. I'll run through each of the doc pages once the build artefact has completed.

@@ -496,9 +496,9 @@ def dual_annealing(func, bounds, args=(), maxiter=1000,
latest minimum found, and ``context`` has value in [0, 1, 2], with the
following meaning:

- 0: minimum detected in the annealing process.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the documentation could do with a little polishing in this section, but that's out of scope of this PR.

@@ -142,13 +142,13 @@ def linprog_terse_callback(res):
feasible solution is sought and the T has an additional row
representing an alternate objective function.
status : int
An integer representing the exit status of the optimization::
An integer representing the exit status of the optimization:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -121,8 +121,8 @@ def minimize(fun, x0, args=(), method=None, jac=None, hess=None,
the Hessian. Available quasi-Newton methods implementing
this interface are:

- `BFGS`;
- `SR1`.
- `BFGS` and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is 'and' required?

Copy link
Contributor Author

@mdhaber mdhaber Nov 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of, because there was a period after SR1, suggesting some sort of attempt to make this a complete sentence. Maybe it's best to throw out the "and" and the period.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


``hessp(x, p, *args) -> ndarray shape (n,)``
hessp(x, p, *args) -> ndarray shape (n,)

where x is a (n,) ndarray, p is an arbitrary vector with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surely some of these should be in backticks (out of scope for this PR)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They render as code blocks because of the preceeding double-colon. Makes a lot of sense, right? : P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdhaber, @andyfaff might be referring to x, (n,), ndarray and p in line 142. But I think it is fine to consider such additional changes as out of scope for this PR.

Copy link
Contributor Author

@mdhaber mdhaber Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, of course. Yeah, there is a lot more to be fixed about the minimize docstring; let's aim this at the appearance of lists.


Constraints for COBYLA, SLSQP are defined as a list of dictionaries.
Each dictionary with fields:

type : str
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be nice just to have new-style objects, and use new-style internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah reviewing the docs does stir those sorts of thoughts.

The Jacobian of `fun` (only for SLSQP).
args : sequence, optional
Extra arguments to be passed to the function and Jacobian.
type : str
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really wish we could use NonlinearConstraint as default and dump the dicts. Anyway, trust-constr accepts new-style constraints so is statement in the Note box correct? https://22669-1460385-gh.circle-artifacts.com/0/html-scipyorg/generated/scipy.optimize.shgo.html#scipy.optimize.shgo

Copy link
Contributor Author

@mdhaber mdhaber Nov 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just wasn't updated when trust-constr was added. I snuck it in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its derivatives (Jacobian, Hessian).
options : dict, optional
Note that by default the tolerance is specified as
``{ftol: 1e-12}``
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 101 should have scipy.optimize.minimize in backticks

Copy link
Contributor Author

@mdhaber mdhaber Nov 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will fix this one, but we could spend days fixing inconsistent use of backticks! We really need to decide what deserves double backticks and what should have just singles. NumPy guidelines say "Enclose variables in double backticks", but where do we draw the line between a "variable" and code? Why is it appropriate for the parameter names to be bold in the parameter list, italicized when we refer to them in the descriptions, and teletype when we actually type them in the IDE? If I had my way, all inline code would be teletype (bold/italicized as needed for emphasis) but without the gray box around, just as Matlab does it

image

But this is not the place : )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -87,14 +87,14 @@ def shgo(func, bounds, args=(), constraints=None, n=100, iters=1, callback=None,
Extra keyword arguments to be passed to the minimizer
``scipy.optimize.minimize`` Some important options could be:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full stop before 'some'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maximum number of function evaluations in the feasible domain.
(Note only methods that support this option will terminate
the routine at precisely exact specified value. Otherwise the
criterion will only terminate during a global iteration)
* f_min
f_min
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type?

Copy link
Contributor Author

@mdhaber mdhaber Nov 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Float, presumably.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9 : Iteration limit reached
Exit modes are defined as follows :

-1 : Gradient evaluation required (g & a)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these aren't being displayed correctly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that. I made them all on separate lines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if the blank lines added here weren't necessary, but it might be an acceptable compromise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still not rendering properly: https://22691-1460385-gh.circle-artifacts.com/0/html-scipyorg/generated/scipy.optimize.fmin_slsqp.html#scipy.optimize.fmin_slsqp

I suspect you need an additional space before -1, to keep the left-most characters in each line aligned.

Copy link
Contributor Author

@mdhaber mdhaber Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgommers rgommers added Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org maintenance Items related to regular maintenance tasks labels Nov 25, 2020
7 : Rank-deficient equality constraint subproblem HFTI
8 : Positive directional derivative for linesearch
9 : Iteration limit reached
Exit modes are defined as follows :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Exit modes are defined as follows :
Exit modes are defined as follows:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Comment on lines +164 to +184
-1 : Gradient evaluation required (g & a)

0 : Optimization terminated successfully

1 : Function evaluation required (f & c)

2 : More equality constraints than independent variables

3 : More than 3*n iterations in LSQ subproblem

4 : Inequality constraints incompatible

5 : Singular matrix E in LSQ subproblem

6 : Singular matrix C in LSQ subproblem

7 : Rank-deficient equality constraint subproblem HFTI

8 : Positive directional derivative for linesearch

9 : Iteration limit reached
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elsewhere we use * in lists like this (e.g. see several cases being edited in this PR). That allows us to avoid the blank lines. Since we're already using that style in several places, we might as well use it here, too.

Suggested change
-1 : Gradient evaluation required (g & a)
0 : Optimization terminated successfully
1 : Function evaluation required (f & c)
2 : More equality constraints than independent variables
3 : More than 3*n iterations in LSQ subproblem
4 : Inequality constraints incompatible
5 : Singular matrix E in LSQ subproblem
6 : Singular matrix C in LSQ subproblem
7 : Rank-deficient equality constraint subproblem HFTI
8 : Positive directional derivative for linesearch
9 : Iteration limit reached
* -1 : Gradient evaluation required (g & a)
* 0 : Optimization terminated successfully
* 1 : Function evaluation required (f & c)
* 2 : More equality constraints than independent variables
* 3 : More than 3*n iterations in LSQ subproblem
* 4 : Inequality constraints incompatible
* 5 : Singular matrix E in LSQ subproblem
* 6 : Singular matrix C in LSQ subproblem
* 7 : Rank-deficient equality constraint subproblem HFTI
* 8 : Positive directional derivative for linesearch
* 9 : Iteration limit reached

(I think this change is in scope for this PR, because it is part of fixing the markup of lists.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... but I'll let y'all decide if that is worthwhile--this is not a blocker for me. Even as it is now, this PR is a big improvement!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elsewhere we use * in lists like this (e.g. see several cases being edited in this PR).

Yes, in some, but definitely not all. The only place where there is any consistency, as far as I've seen is within linprog, where bullets are not used.

Since there is not yet consensus about which style is best, I wrote above that I'd decided not to add or remove bullets elsewhere, so I'd like to leave this one alone.

@mdhaber mdhaber closed this Feb 1, 2021
@mdhaber mdhaber deleted the remove_unnecessary_indent branch February 1, 2021 07:50
@mdhaber mdhaber restored the remove_unnecessary_indent branch February 1, 2021 07:51
@mdhaber mdhaber reopened this Feb 1, 2021
@tupui tupui mentioned this pull request Mar 22, 2021
@mdhaber mdhaber closed this Apr 9, 2021
@mdhaber mdhaber reopened this Apr 9, 2021
@mdhaber
Copy link
Contributor Author

mdhaber commented May 12, 2021

@WarrenWeckesser do we still want this? With the current template, Sphinx does not render block quotes anymore, so this is less important.

@github-actions github-actions bot added scipy.optimize and removed Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org labels Sep 3, 2021
@mdhaber
Copy link
Contributor Author

mdhaber commented Dec 6, 2021

There were some useful improvements to root finder documentation here (e.g. _root_broyden2_doc), but that was not really the point of this PR. Closing because there are bigger fish to fry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.optimize
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants