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: optimize, special, signal: Use custom warnings instead of print statements #15259
Conversation
elif method in ('fmin', 'fmin_powell'): | ||
kwargs['maxiter'] = 3500 |
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'd suggest only changing test warnings to Warning
objects in this PR rather than adding new warnings.
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'm sorry. Could you elaborate it?
@@ -1238,6 +1240,15 @@ def test_respect_maxiter(self, method): | |||
if method == 'SLSQP': | |||
assert sol.status == 9 # Iteration limit reached | |||
|
|||
@pytest.mark.parametrize('method', ['Nelder-Mead', 'Powell']) |
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.
Are these the only two methods affected by this PR? If others are affected, consider testing them, too.
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.
Thanks. I missed 'fmin', 'fmin_powell', brute
, so I added tests for these.
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.
Thanks @AtsushiSakai. Once it's done we should close #14050
scipy/optimize/_optimize.py
Outdated
@@ -887,12 +887,12 @@ def _minimize_neldermead(func, x0, args=(), callback=None, | |||
warnflag = 1 | |||
msg = _status_message['maxfev'] | |||
if disp: | |||
print('Warning: ' + msg) | |||
warnings.warn(msg, UserWarning) |
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.
We should also add some stacklevel
(2 or 3 usually depending on where it is). Cf #1953 (comment)
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.
+1. Good call. I always forget about that.
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.
Same! I understood this one quite recently.
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 added.
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.
Thanks for the changes so far, @AtsushiSakai! Last few suggestions. Once they are applied, I think this will be ready from my side!
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.
signal
and special
chages look good to me now. Thanks, @AtsushiSakai! @mdhaber @tupui It'd be great if one of you could verify optimize
tests and merge.
afd94df
to
0505c75
Compare
Co-authored-by: Pamphile Roy <roy.pamphile@gmail.com>
Co-authored-by: Tirth Patel <tirthasheshpatel@gmail.com>
Co-authored-by: Tirth Patel <tirthasheshpatel@gmail.com>
Co-authored-by: Tirth Patel <tirthasheshpatel@gmail.com>
343e8d5
to
15ca55e
Compare
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.
LGTM, going forward. Thanks again everyone 😃
Reference issue
Fix #1953
What does this implement/fix?
Use custom warnings instead of print statements and add tests.