-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
FIX: Avoid use of bare except #9133
Conversation
signal.medfilt(None) | ||
except: | ||
pass | ||
signal.medfilt(None) |
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 raises
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.
Tests pass on my machine and CIs (except for an unrelated error). Do I need to run some special testing mode?
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.
For the failed example, can you replace >>> print(res.x)
by >>> res.x
, and copy-paste the output?
It's just that the doctest checker does not understand the way numpy arrays are printed. Not sure why the error surfaces with this PR though.
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.
Ok, please disregard, I apparently misremember...
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.
@ev-br I made a commit to try what you asked and this was the output:
File "doc/source/tutorial/optimize.rst", line 699, in /home/travis/build/scipy/scipy/tools/../doc/source/tutorial/optimize.rst
Failed example:
res.x
Expected:
np.array([0.41494418 0.17011164])
Got:
array([0.41494475, 0.1701105 ])
I'm just going to put a # may vary
in there for now
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.
https://github.com/scipy/scipy/compare/master...ev-br:doctest_tutorial?expand=1 should fix it, I think.
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 I tried a variant of this and it didn't work, but I'll open a PR to master
for this. It's happening in other PRs, now, too, so I think mine was somehow just unfortunately timed to make it seem like it was due to my changes. So I'm going to revert the ones here and hopefully we can merge.
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.
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.
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've just placed a PR
All green now that Ready for review/merge from my end |
Looks good to me. |
LGTM too and all green, merging. |
Follow-up to #9129. This PR:
E722
from the list of ignores intox.ini
.except:
in tests with cleanerpytest.raises
callsexcept:
s in our code withexcept Exception:
that seemed reasonable. In a few places there was anexcept: ...; raise
pattern to close some other object, to which I added# noqa: E722
. This works forpydocstyle
andflake8
-style checkers, can't remember what SciPy uses for style checking so I'll wait to see what the CIs say about it.