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
MNT: Suppress LineSearchWarning's in scipy.optimize tests #4570
Conversation
warnings.simplefilter('ignore', RuntimeWarning) | ||
s, fc, gc, fv, ofv, gv = ls.line_search_wolfe2(f, fprime, x, p, | ||
g0, f0, old_f, | ||
amax=smax) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could only filter LineSearchWarning
(from scipy.optimize.linesearch
) because plain RuntimeWarning
s are not expected here IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it after making the commit. I changed it now.
67d6485
to
a4a08c3
Compare
x = optimize.fmin_bfgs(func, x0, fprime, disp=False) | ||
with warnings.catch_warnings(): | ||
warnings.simplefilter('ignore', LineSearchWarning) | ||
x = optimize.fmin_bfgs(func, x0, fprime, disp=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the warning should not be filtered at this level, because BFGS actually uses a wrapper to line_search_wolfe2, the function raising the warning. That is _line_search_wolfe12
in optimize.py, which handles the fact that the line search algorithm may not converge. So, IMO, the warning should be filtered within the wrapper, and it would hence not appear in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the only valid filtering in this file is for direct calls to line_search_wolfe2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if we filter warnings in _line_search_wolfe12
a user won't see them when this wrapper is called. It looks like undesired behaviour, doesn't it?
I might be confused with LineSearchWarning
vs _LineSearchError
. The former is raised in line_search_wolfe2
when derphi_star
is None
, the latter is raised in _line_search_wolfe12
when alpha_star
is None
.
As far as I understand we still want to give a warning (but not an error), when derphi_star
is None
. Although currently I have no idea about algorithm details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nikolay Mayorov wrote:
But if we filter warnings in |_line_search_wolfe12| a user won't see
them when this wrapper is called. It looks like undesired behaviour,
doesn't it?
No, because this wrapper handles the case of line_search_wolfe2
returning None and then raise an exception (which is then catched in the
caller). So there's no point warning the user about something they have
no control on.
I might be confused with |LineSearchWarning| vs |_LineSearchError|. The
former is raised in |line_search_wolfe2| when |derphi_star| is |None|,
the latter is raised in |_line_search_wolfe12| when |alpha_star| is |None|.As far as I understand we still want to give a warning (but not an
error), when |derphi_star| is |None|. Although currently I have no idea
about algorithm details.
I think we want a warning only when line_search_wolfe2 is called
directly by the user, to let them know about converge issue, although
the function returns something (this behavior must be kept for backwards
compatibility).
a4a08c3
to
267e1d8
Compare
@dlax, I think I understood what you were saying. I updated the commit (I think more than one is not appropriate here). |
MNT: Suppress LineSearchWarning's in scipy.optimize tests
Merged, thanks @nmayorov ! |
I tried to fix #4554.
Tell me if I did it correctly.