DOC: indicate possibility that line_search returns some values that are None

[argriffing]

Partially addresses #4257.

[dlax]

How about raising an exception instead?

[argriffing]

How about raising an exception instead?

Yes that would make more sense. This PR is just intended to document the current behavior.

[dlax]

argriffing a écrit :

How about raising an exception instead?

Yes that would make more sense. This PR is just intended to document the
current behavior.

The thing is that changing the current behavior to raise an exception
might be considered backwards incompatible. Though, it seems most usages
of line search functions within scipy actually check for None and then
raise a specific exception (_LineSearchError(RuntimeError)). So making
this "official" would make sense IMO. Alternatively a warning could be used.
Other opinions?

[rgommers]

I can easily imagine users that ran into None before having writting code to work around it (maybe trying another starting point), so a clear warning may be preferable to an exception.

[dlax]

I guess I'd also prefer a (documented) warning to be raised after all. Also "something has gone wrong" is not very informative, just saying that the line search algorithm did not converge might be better.

[argriffing]

I have a bias against adding warnings because of the drawbacks of design by committee. But in this case it might make sense. The meaning of the warning would be "Your optimization has failed, but for backwards compatibility this is a warning instead of an exception, and some of the return values of the line search function will be None." but the warning text should be translated into something more bland.

[argriffing]

Added the warning and improved the explanation for None return values.

[dlax]

Thanks @argriffing. Sorry for nitpicking again but, wouldn't a plain RuntimeWarning be more appropriate and sufficient? There is already a few of those in scipy for "similar" issues.

[argriffing]

@dlax I used RuntimeWarning originally, but it is transformed into an error on my system, possibly because I'm using a development version of numpy. I tried changing to OptimizeWarning as a base class, but there was some kind of import problem, maybe a cyclic import. Then I switched to a base class of UserWarning because that is the base class of OptimizeWarning.

[dlax]

argriffing a écrit :

@dlax https://github.com/dlax I used |RuntimeWarning| originally, but
it is transformed into an error on my system, possibly because I'm using
a development version of numpy. I tried changing to |OptimizeWarning| as
a base class, but there was some kind of import problem, maybe a cyclic
import. Then I switched to a base class of |UserWarning| because that is
the base class of |OptimizeWarning|.

I'd say that UserWarning (base class for warnings generated by user
code) is not semantically appropriate in this case. RuntimeWarning (Base
class for warnings about dubious runtime behavior) is probably more.

[argriffing]

@dlax Currently I think the scipy tests never raise uncaught RuntimeWarnings. One reason is that these warnings (along with DeprecationWarning) are promoted to Exceptions in certain environments (e.g. possibly numpy development versions). Changing the base class from UserWarning to RuntimeWarning would introduce some uncaught exceptions that may not be completely straightforward to deal with in terms of modification of the scipy unit tests. Also, your argument favoring RuntimeWarning over UserWarning as a base class would apply not only to the LineSearchWarning but also to OptimizeWarning, right?

[argriffing]

OptimizeWarning was introduced in #217 with the UserWarning base class.

[dlax]

IIRC, OptimizeWarning concerns user code, so it's appropriate for it to be a UserWarning.

[argriffing]

@dlax At least the first of the following couple of uses does not particularly seem to concern user code, except in the loose sense that all optimization concerns user code at some level.
https://github.com/scipy/scipy/blob/maintenance/0.15.x/scipy/optimize/minpack.py#L602
https://github.com/scipy/scipy/blob/maintenance/0.15.x/scipy/optimize/optimize.py#L127

[dlax]

argriffing wrote:

@dlax https://github.com/dlax At least the first of the following
couple of uses does not particularly seem to concern user code, except
in the loose sense that all optimization concerns user code at some level.
https://github.com/scipy/scipy/blob/maintenance/0.15.x/scipy/optimize/minpack.py#L602
https://github.com/scipy/scipy/blob/maintenance/0.15.x/scipy/optimize/optimize.py#L127

So there's an inconsistency.

Anyways, I still fail to see the real problem with using a RuntimeWarning.

[argriffing]

Anyways, I still fail to see the real problem with using a RuntimeWarning.

I agree that there is not a real problem with using a RuntimeWarning. My intention with this low-effort PR was to quickly update the line_search docs. If others would like to improve the line_search function itself rather than just documenting its current possibly sub-optimal behavior then that would be awesome and we could close this PR!

[rgommers]

Since the non-convergence is usually due to the user specifying something incorrectly, a subclass of UserWarning doesn't seem completely inappropriate. I'm fine with either merging this as is or changing to RuntimeWarning.

[argriffing]

Changed from UserWarning to RuntimeWarning. It seems to pass TravisCI so I guess it's OK.

[dlax]

Merged, thanks @argriffing !

[argriffing]

FWIW this PR causes test errors on unreleased numpy. I think this is because they set some kind of global flags to escalate warnings to errors for more stringent testing during development than in released versions.

[rgommers]

This indicates that there are now warnings raised by tests added here which should be filtered out. The common pattern for that is:

 with warnings.catch_warnings():
    warnings.simplefilter('ignore', UserWarning)
    function_call_that_gives_expected_warning()