Opened 4 years ago

Closed 4 years ago

#23062 closed enhancement (fixed)

Document disp option of minimize

Reported by: mforets Owned by:
Priority: major Milestone: sage-8.0
Component: numerical Keywords: documentation, optimization
Cc: dimpase, dcoudert Merged in:
Authors: Marcelo Forets Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 83e9a04 (Commits, GitHub, GitLab) Commit: 83e9a04d74485488739204f758baebe4f37989f4
Dependencies: Stopgaps:

Status badges

Description

The disp keyword of the top-level minimize function does not appear in the docstring. Moreover, it is currently set up to be verbose by default (don't think is a good idea), hence displays a message similar to:

Optimization terminated successfully.
         Current function value: 0.000000
         Iterations: 28
         Function evaluations: 35
         Gradient evaluations: 35

Change History (18)

comment:1 Changed 4 years ago by mforets

homework:

  • #14607 for the verbose issue
  • #14493 to improve docs about the format of the obj function accepted
  • #6592 for a similar issue with minimize_constrained

comment:2 Changed 4 years ago by mforets

  • Authors set to Marcelo Forets
  • Branch set to u/mforets/23062
  • Cc dimpase dcoudert added
  • Commit set to dec36b7b1e032addaa67c7e0035433b441e6cbc9

i'm seeking your opinion about: verbose mode by default yes or not?

as stated above, i'm +1 for no convergence message by default.


New commits:

dec36b7add disp input to docstring

comment:3 Changed 4 years ago by dimpase

If you want to make disp=False the default, it should be done via the deprecation route, as it changes the output.

comment:4 follow-up: Changed 4 years ago by dcoudert

I have no opinion for this change since I never use these methods. Note that I find the keyword verbose more usual than disp.

comment:5 in reply to: ↑ 4 Changed 4 years ago by dimpase

Replying to dcoudert:

I have no opinion for this change since I never use these methods. Note that I find the keyword verbose more usual than disp.

name disp comes from the backend, it is not something we could change, it seems.

comment:6 Changed 4 years ago by dimpase

more precisely, verbose would be nice and easy to have, assuming the backend does not use it for something else. Otherwise it looks to complicated to do this change.

comment:7 Changed 4 years ago by mforets

from that side i think it can be renamed inside minimize; it is a true/false flag that is passed to SciPy?'s optimize methods.

(fprime has been renamed to gradient, for instance.)

inside the numerical module, the verbose keyword is currently used in:

  • interactive simplex method (inject_variables)
  • knapsack problem

OTOH, disp is used only here, on minimize.

comment:8 Changed 4 years ago by dimpase

OK, let's go with verbose instead.

comment:9 Changed 4 years ago by git

  • Commit changed from dec36b7b1e032addaa67c7e0035433b441e6cbc9 to be7fcf5fd3d43025f90c567ddfe3f4ef47c167d6

Branch pushed to git repo; I updated commit sha1. New commits:

ff2f590change disp -> verbose keyword
1f1fecffix keyword in doctests
be7fcf5add rename keyword deprecation warning

comment:10 Changed 4 years ago by git

  • Commit changed from be7fcf5fd3d43025f90c567ddfe3f4ef47c167d6 to 469bd82e99d77842bad557889787eb72abc4fabb

Branch pushed to git repo; I updated commit sha1. New commits:

469bd82set default -> False, and update doctests

comment:11 Changed 4 years ago by mforets

change are done ==> essentially for review..

although i doubted is if it's the good deprecation warning that i chose. from the list in the Guide, there is no explicit mention about "changing the output".

comment:12 follow-up: Changed 4 years ago by mforets

here is another thing with this function: suppose you choose algorithm='ncg'. then, the line with optimize.fmin_ncg does not get executed if f is a Python function?! (not even an exception?)

comment:13 Changed 4 years ago by git

  • Commit changed from 469bd82e99d77842bad557889787eb72abc4fabb to 540bdeac0538207c5a0d1bf3613a91dbdba9e41d

Branch pushed to git repo; I updated commit sha1. New commits:

46e3497add missing algorithm descriptions and wiki link
f0373a0update verbose in docstring
540bdeaadd complementary note

comment:14 in reply to: ↑ 12 Changed 4 years ago by mforets

Replying to mforets:

here is another thing with this function: suppose you choose algorithm='ncg'. then, the line with optimize.fmin_ncg does not get executed if f is a Python function?! (not even an exception?)

i had closer look into this, and created #23074

comment:15 Changed 4 years ago by git

  • Commit changed from 540bdeac0538207c5a0d1bf3613a91dbdba9e41d to 83e9a04d74485488739204f758baebe4f37989f4

Branch pushed to git repo; I updated commit sha1. New commits:

83e9a04remove pair of backticks (all or nothing)

comment:16 Changed 4 years ago by mforets

  • Status changed from new to needs_review

no further changes expected (from my side) ==> needs review

comment:17 Changed 4 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

Looks good to me, thanks!

comment:18 Changed 4 years ago by vbraun

  • Branch changed from u/mforets/23062 to 83e9a04d74485488739204f758baebe4f37989f4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.