-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
MAINT: change neldermead prints to warnings #14050
Conversation
Thank you for your first PR! Great start 😃 Feel free to ping me for any help! |
Thanks @tupui! I got the second case working. Could you take a look? |
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.
This looks good, I just have very minor comments.
def test_warn_neldermead(self): | ||
with pytest.warns(UserWarning, match=r'Maximum number of function'): | ||
func = lambda x: np.sum(x ** 2) | ||
p0 = [0.15746215, 0.48087031, 0.44519198, 0.4223638, 0.61505159, |
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.
Why these specific values?
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 agree - we could use much simpler initial values with a really low function evaluation limit. Do you agree?
with pytest.warns(UserWarning, match=r'Maximum number of iterations'): | ||
x0 = np.zeros(10) | ||
func = optimize.rosen | ||
optimize.fmin(func, x0, maxiter=5) |
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.
Could you make this work with _minimize_neldermead
? It would make more sense to call this instead.
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 agree with consistency, but actually, what do you think about having both of these tests use the minimize
interface, since we are interested in what users will see when they use the recommended public function?
@@ -834,12 +834,12 @@ def _minimize_neldermead(func, x0, args=(), callback=None, | |||
warnflag = 1 | |||
msg = _status_message['maxfev'] | |||
if disp: | |||
print('Warning: ' + msg) | |||
warnings.warn(msg, UserWarning, 3) |
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.
It would be good to see if 3
for the stacklevel is really needed.
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.
Actually here is the relevant part of the Python doc: https://docs.python.org/3/library/warnings.html#warnings.warn
So I guess in this case the default value 1
is ok.
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.
@tupui but don't we want it to show up as a warning from the function that the user called rather than private _minimize_neldermead
?
Hey @lokijuhy just checking in. Can I help you with something? |
@lokijuhy friendly reminder. Let me know if you still intend to work on this. |
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.
@tupui I'll go ahead and finish this up, but a few questions so that I head in the right direction.
@@ -834,12 +834,12 @@ def _minimize_neldermead(func, x0, args=(), callback=None, | |||
warnflag = 1 | |||
msg = _status_message['maxfev'] | |||
if disp: | |||
print('Warning: ' + msg) | |||
warnings.warn(msg, UserWarning, 3) |
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.
@tupui but don't we want it to show up as a warning from the function that the user called rather than private _minimize_neldermead
?
def test_warn_neldermead(self): | ||
with pytest.warns(UserWarning, match=r'Maximum number of function'): | ||
func = lambda x: np.sum(x ** 2) | ||
p0 = [0.15746215, 0.48087031, 0.44519198, 0.4223638, 0.61505159, |
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 agree - we could use much simpler initial values with a really low function evaluation limit. Do you agree?
with pytest.warns(UserWarning, match=r'Maximum number of iterations'): | ||
x0 = np.zeros(10) | ||
func = optimize.rosen | ||
optimize.fmin(func, x0, maxiter=5) |
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 agree with consistency, but actually, what do you think about having both of these tests use the minimize
interface, since we are interested in what users will see when they use the recommended public function?
Reference issue
Address partially gh-1953.
What does this implement/fix?
Convert all
print('Warning...')
statements to warnings withinscipy/optimize/optimize.py::minimize_neldermead
.