Misc fixes pointed out by Eclipse PyDev static code analysis #2942

Closed
wants to merge 9 commits into
from

Projects

None yet

4 participants

@cdeil
Contributor
cdeil commented Sep 29, 2013

Here's fixes for a few scipy issues pointed out by my editor (Eclipse PyDev).
I'm not familiar with the code and didn't test, so please check the changes carefully.

There's two more that I think might be real issues where I don't know what the correct fix is:

Description Resource    Path    Location    Type
Undefined variable: get_psolve  iterative.py    /scipy/scipy/sparse/linalg/isolve   line 198    PyDev Problem
Undefined variable: ct  hb.py   /scipy/scipy/io/harwell_boeing  line 257    PyDev Problem

https://coveralls.io/files/59355318#L199
https://coveralls.io/files/59355112#L258

All of these issues are not covered by unit tests.

@coveralls

Coverage Status

Coverage remained the same when pulling c68403e on cdeil:cleanup into ce9d9d1 on scipy:master.

@larsmans larsmans commented on the diff Oct 2, 2013
scipy/spatial/kdtree.py
@@ -238,19 +238,19 @@ def __init__(self, data, leafsize=10):
class node(object):
if sys.version_info[0] >= 3:
def __lt__(self, other):
- id(self) < id(other)
+ return id(self) < id(other)
@larsmans
larsmans Oct 2, 2013 Contributor

I guess a unit test for this change would be in order.

@rgommers
Member
rgommers commented Oct 4, 2013

Thanks @cdeil, those fixes all look good. Unit tests for the kdtree and fitpack changes would indeed be very helpful as @larsmans points out.

@rgommers
Member
rgommers commented Oct 4, 2013

From a quick check it also looks like the node comparisons are missing from ckdtree.

@cdeil
Contributor
cdeil commented Oct 13, 2013

@larsmans @rgommers These issues were just pointed out by a static code analysis ... I've never used these parts of scipy and it would take me quite some time to figure out how to add the unit tests you suggest, which I don't have at the moment.
Maybe you can quickly add the extra unit tests or ping the maintainers of the relevant modules to do it?

@larsmans
Contributor

I've sent to a PR to @cdeil's repo with a test for the KDTree stuff.

@cdeil
Contributor
cdeil commented Oct 13, 2013

Thanks @larsmans ... merged your PR into this one.

@coveralls

Coverage Status

Coverage remained the same when pulling c4e0be7 on cdeil:cleanup into ce9d9d1 on scipy:master.

@rgommers
Member

Added dblint test in 860bf8e and merged this PR in e572003. Thanks @cdeil and @larsmans.

@rgommers rgommers closed this Oct 14, 2013
@thouis thouis pushed a commit to thouis/scipy that referenced this pull request Oct 30, 2013
@rgommers rgommers Merge branch 'pr/2942' into master. e572003
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment