TST: improve signal.bsplines test coverage #9035
Conversation
signal.bsplines test scipy#6835
…atch-1 Add files via upload
scipy/signal/bsplines.py
Outdated
| @@ -133,6 +133,8 @@ def gauss_spline(x, n): | |||
| """Gaussian approximation to B-spline basis function of order n. | |||
| """ | |||
| signsq = (n + 1) / 12.0 | |||
| if signsq == 0: | |||
There was a problem hiding this comment.
This can only happen when n = -1 and doesn't make sense anyways. And the error to raise is wrong input argument not zero by division anyways.
There was a problem hiding this comment.
Fair point, I wrote this to conform to the tests, but you are correct that that makes more sense. I'll fix this.
There was a problem hiding this comment.
This is still not quite right. Rather than doing input validation for n, I'd just remove these two lines again and instead just document that n needs to be an integer >= 0.
There was a problem hiding this comment.
You could also put in a reference to https://pdfs.semanticscholar.org/360c/77005e0ac26fa0aecbea214f37609bb52414.pdf, that contains the math. It has sqrt(n/12) rather than sqrt((n+1)/12)), but I don't want to read the paper in enough detail to sort that out. Just throwing the link into the docstring under References would be helpful though.
scipy/signal/tests/test_bsplines.py
Outdated
| data_array_complex = 0.1*data_array_complex | ||
| result_array_complex = array( | ||
| [[0.40882362, 0.41021151, 0.40886708, 0.40905103], | ||
| [0.40829477, 0.4102123, 0.40966097, 0.40939871], |
There was a problem hiding this comment.
PEP8 : One extra whitespace after second entry
|
Quick comment for future PRs: please write a descriptive title --- there aren't actually "new bsplines" in this PR. |
|
Will do, sorry about that. I focused on the description and forgot the title. I've addressed the comments. |
rgommers
changed the title
rgommers
added
scipy.signal
maintenance
|
The |
|
Tests have been fixed, thanks for the tip. |
|
The test classes should not inherit from TestCase, since we are using
pytest nowadays. Instead, write `class TestBSplines(object):` etc.
|
|
Similarly due to pytest, the `if __main__: run_module_suite()` should
be removed.
|
|
I'm not sure how the changes I made could relate to these errors. They seem to come from a completely unrelated module, and those tests passed before my most recent commit. |
Indeed unrelated, can be ignored for this PR. There's something weird going on, those tests regularly keep failing. All LAPACK related it looks like. Furthermore we also need to tackle the ridiculously long pytest output - 2500 lines for 7 test failures is unhelpful (to put it mildly). |
|
LGTM aside for the |
|
LGTM now. Test failure is unrelated. Merging. Thanks @vyasr! |
This pull request is based on pull request #6846 and resolves the outstanding issues there.