BUG: Issue 5354 - Fixed segmentation fault when clipping complex arrays #5386

maniteja123 · 2014-12-22T18:03:13Z

BUG:An attempt to fix Issue #5354 .
The NULL dereference before checking whether the pointer is a NULL pointer is causing a segmentation fault. I am not sure whether the output coming now is the right one. Please do clarify anything else is wrong except the segmentation fault.

F_min
(-256+0j)
F.clip(F_min)
array([[ 8.29056000e+06 +0.j , 2.56000000e+02+358.28772462j,
-2.56000000e+02 +0.j , ...,
2.56000000e+02+861.73946261j, -2.56000000e+02 +0.j ,
2.56000000e+02-358.28772462j],
[ 0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j , ...,
0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j ],
[ 0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j , ...,
0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j ],
...,
[ 0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j , ...,
0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j ],
[ 0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j , ...,
0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j ],
[ 0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j , ...,
0.00000000e+00 +0.j , 0.00000000e+00 +0.j ,
0.00000000e+00 +0.j ]])

rgommers · 2014-12-22T18:06:41Z

This one looks better already:)

rgommers · 2014-12-22T18:10:06Z

@maniteja123 this segfaults on linux as well, so I assume you tested it and the fix works for you? If so, can you add the example of gh-5385 as a regression test? You can put it in numpy/core/tests/test_regression.py.

rgommers · 2014-12-22T18:10:32Z

And like I commented on the other PR, just delete these two lines instead of commenting them out.

rgommers · 2014-12-22T18:12:20Z

Side note: C++ style (//) comments are not supposed to be used in numpy, see the C style guide: https://github.com/numpy/numpy/blob/master/doc/C_STYLE_GUIDE.rst.txt#c-dialect

maniteja123 · 2014-12-22T18:35:20Z

@rgommers Thanks for the advice. I have deleted the files and pushed it to the branch. Also, from next time I will make sure to follow the coding style guidelines. I was confused whether to delete the lines or not, so I commented them.

maniteja123 · 2014-12-22T18:44:25Z

I have tested it by changing the file in inplace build and tested the example used in the issue #5354 to check whether seg fault is occurring or not . I have looked at the test_regressiontest.py file, but I am not particularly sure how to define the test functions and then assert statements. I have read the https://github.com/numpy/numpy/blob/master/doc/TESTS.rst.txt#writing-your-own-tests file, but how should I use the assert statements for the example in #5354 ?

rgommers · 2014-12-22T18:48:16Z

You don't have to use an assert in this case. Just literally use the example that crashes now. If the issue is fixed the test will then pass, if not the whole test suite will crash.

argriffing · 2014-12-22T18:48:47Z

I assume this TravisCI failure is unrelated?

rgommers · 2014-12-22T18:50:31Z

Indeed looks unrelated. I've restarted the test.

maniteja123 · 2014-12-22T18:53:17Z

Oh, then I will just add the corresponding program in a function and run numpy.test() ?

maniteja123 · 2014-12-22T18:57:27Z

@argriffing So, should I add testing ex 8000 ample to the test_multiarray.py file ? Also it looks like all PR related checks are added to test_regression.py file.

argriffing · 2014-12-22T18:59:48Z

@maniteja123 I was wrong in that comment I'd deleted. There was a function in that file called test_clip but it was not actually testing clip. I agree that test_regression.py looks like a good place to put it.

maniteja123 · 2014-12-22T19:11:49Z

Ran 5679 tests in 899.856s

OK (KNOWNFAIL=5, SKIP=8)
<nose.result.TextTestResult run=5679 errors=0 failures=0>

This is the result I got after running numpy.test()
Shall I also push the change I did in the test_regression.py ?

rgommers · 2014-12-22T20:19:03Z

@maniteja123 you please push that change with the regression test.

rgommers · 2014-12-22T20:19:30Z

And I'll have a look at what's going on with that USE_WHEEL test.

maniteja123 · 2014-12-22T20:22:24Z

@rgommers I have pushed the change to the branch. I am not sure if this is what was expected output and also the test suite is correctly written. Please do have a look :)

rgommers · 2014-12-22T20:28:22Z

numpy/core/tests/test_regression.py

@@ -2103,6 +2103,17 @@ def __eq__(self, other):
        assert_equal(np.int32(10) == x, "OK")
        assert_equal(np.array([10]) == x, "OK")

+    def test_fft(self):
+	#PR 5386


Nitpick: needs a space after # and I'd refer to the bug report. So:

# Check segfault reported in gh-5354 doesn't occur anymore.

rgommers · 2014-12-22T20:44:45Z

Looks like a minor thing:

ERROR: Failure: TabError (inconsistent use of tabs and spaces in indentation (test_regression.py, line 2108))

You cannot mix tabs and spaces within Python files, it has to be spaces-only. You can replace them now by hand, but you should also look at setting up your text editor or IDE so it always replaces tabs with 4 spaces for all Python files. Any good editor will have an option to do this.

rgommers · 2014-12-22T20:48:10Z

The USE_WHEEL failure is unrelated to this PR, and quite odd. The testlog should look like:

pip install --pre --upgrade --find-links dist numpy
Unpacking ./dist/numpy-1.10.0.dev4198570-cp27-none-linux_x86_64.whl
Installing collected packages: numpy
  Found existing installation: numpy 1.9.1
    Uninstalling numpy:
      Successfully uninstalled numpy
Successfully installed numpy

However for this PR it says:

pip install --pre --upgrade --find-links dist numpy
Requirement already up-to-date: numpy in /home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages
...
NPY_RELAXED_STRIDES_CHECKING set, but not active.

So somehow TravisCI is mixing up two builds.

rgommers · 2014-12-22T20:53:05Z

Looks like pip version just changed from 1.5.6 to 6.0, so the failure with a wheel is likely a regression in`pip``.

maniteja123 · 2014-12-22T20:55:55Z

@rgommers
As for the tabs, I have now manually used the 4 spaces and have pushed the changes. I am using Gedit right now. There was an option to insert spaces instead of tabs. I have put the tab width to 4.

Regarding travis build error, is there any chance of me using the inplace build causing the trouble there ?

Also where do I get to learn these commenting styles in the discussions on github like embedding links to source code (in particular, some lines in github repo ), highlighting key points or putting code snippets ?

argriffing · 2014-12-22T21:03:38Z

pip version just changed from 1.5.6 to 6.0

Nice find. The earlier pip version bump also caused some complications if I remember correctly.

juliantaylor · 2014-12-24T15:26:49Z

the regression test can be removed

the self.clip functions are wrapper functions around the real array clip used for the testing here, they do not have the same interface with default arguments, setting None explicitly is correct here then.

maniteja123 · 2014-12-24T15:30:46Z

I gave a as np.ones(10, dtype=np.complex)
This is the failed test response. Please clarify.

FAIL: test_clip_complex (test_numeric.TestClip)

Traceback (most recent call last):
File "/home/maniteja/FOSS/numpy/numpy/core/tests/test_numeric.py", line 1187, in test_clip_complex
assert_array_strict_equal(ac, act)
File "/home/maniteja/FOSS/numpy/numpy/core/tests/test_numeric.py", line 1051, in assert_array_strict_equal
assert_array_equal(x, y)
File "/home/maniteja/FOSS/numpy/numpy/testing/utils.py", line 739, in assert_array_equal
verbose=verbose, header='Arrays are not equal')
File "/home/maniteja/FOSS/numpy/numpy/testing/utils.py", line 665, in assert_array_compare
raise AssertionError(msg)
AssertionError:
Arrays are not equal

(mismatch 100.0%)
x: array([ 1.+0.j, 1.+0.j, 1.+0.j, 1.+0.j, 1.+0.j, 1.+0.j, 1.+0.j,
1.+0.j, 1.+0.j, 1.+0.j])
y: array([None, None, None, None, None, None, None, None, None, None], dtype=object)

juliantaylor · 2014-12-24T15:34:41Z

how does the test look like?
the assert means the result is not what you are expecting, either because of a bug in the test or the actual function tested is broken (which should not be the case)

maniteja123 · 2014-12-24T15:35:33Z

def test_clip_complex(self):
    #Test native complex input without explicit min/max ie, either min=None or max=None
    #a   = 2 * self._generate_data_complex(self.nr, self.nc)
    a   = np.ones(10, dtype=np.complex)
    m   = a.min()
    M   = None
    ac  = self.fastclip(a, m, M)
    act = self.clip(a, m, M)
    assert_array_strict_equal(ac, act)

maniteja123 · 2014-12-24T15:36:27Z

I understand that it is a wrapper function. np.clip(a, a.min()) is raising a TypeError while a.clip(a.min()) isn't. But what I am not able to understand is why it is returning a ndarray of object type, while it works fine with np.clip or a.clip.
In the test output, the output we get by using the fastclip is correct, while the one using clip is incorrect

juliantaylor · 2014-12-24T15:45:12Z

self.clip uses a different method to clip which apparently does not work with None
you can just put in the result you expect instead of calling self.clip

maniteja123 · 2014-12-24T15:51:43Z

Oh, I see https://github.com/numpy/numpy/blob/master/numpy/core/tests/test_numeric.py#L1078 , which is a slow clip. I didn't think of that :)

maniteja123 · 2014-12-24T15:52:33Z

Now, with a predefined example, the test suite is running fine. I will remove the regression test and push a commit to the branch

maniteja123 · 2014-12-24T17:14:16Z

I have pushed the commit and the travis build also is successful :).

On a side note, would it be preferable to make the defined slow clip function to take care of the cases when min or max are specified as None ?

rgommers · 2014-12-28T19:23:42Z

@maniteja123 another comment about style issues: almost every line contains a PEP8 violation:

numpy/core/tests/test_numeric.py:1180:9: E265 block comment should start with '# '
numpy/core/tests/test_numeric.py:1180:80: E501 line too long (91 > 79 characters)
numpy/core/tests/test_numeric.py:1181:10: E221 multiple spaces before operator
numpy/core/tests/test_numeric.py:1182:10: E221 multiple spaces before operator
numpy/core/tests/test_numeric.py:1183:10: E221 multiple spaces before operator
numpy/core/tests/test_numeric.py:1184:11: E221 multiple spaces before operator
numpy/core/tests/test_numeric.py:1185:25: E201 whitespace after '['
numpy/core/tests/test_numeric.py:1185:80: E501 line too long (113 > 79 characters)
numpy/core/tests/test_numeric.py:1186:43: W291 trailing whitespace
numpy/core/tests/test_numeric.py:1187:1: W293 blank line contains whitespace

You can run https://pypi.python.org/pypi/pep8 over your code and make sure it's clean. Note that it will report many other things in numpy which are there from long ago - but we try to not add new style problems....

rgommers · 2014-12-28T19:32:51Z

numpy/core/tests/test_numeric.py

+        m   = a.min()
+        M   = None
+        ac  = self.fastclip(a, m, M)
+        act = np.array([ 1.+0.j,  1.+0.j,  1.+0.j,  1.+0.j,  1.+0.j,  1.+0.j,  1.+0.j, 1.+0.j,  1.+0.j,  1.+0.j])


This line isn't needed, act == a.

rgommers · 2014-12-28T19:35:29Z

@maniteja123 if you're comfortable with git, you may want to try to squash all commits (git rebase -i master and then change pick to squash in all but the first commit). If not, we'll do that before committing.

rgommers · 2014-12-28T19:36:14Z

I think content-wise this can go in.

maniteja123 · 2014-12-30T05:34:37Z

Sorry, I was out of station yesterday. I have taken care of PEP8 issues (I was unaware of the new rules, I just followed the previous guidelines ), also changed the comment for the regression test, mentioned the issue number 5354 and squashed the commits. I am not that acquainted with git, but am trying to get habituated to using git.
Just asking out of curiosity, doesn't these PEP issues show up in Travis build ?

jaimefrio · 2014-12-30T08:25:10Z

numpy/core/tests/test_numeric.py

+        a = np.ones(10, dtype=np.complex)
+        m = a.min()
+        M = None
+        ac = self.fastclip(a, m, M)


I still think we should have the symmetrical test here as well, i.e. test both fastclip(a, a.min(), None) and fastclip(a, None, a.max()).

Makes sense.

I have added two symmetric tests as suggested by @jaimefrio . I have also pushed the changes.

…c regression test

BUG: Issue 5354 - Fixed segmentation fault when clipping complex arrays

jaimefrio · 2014-12-30T11:49:07Z

Thanks, @maniteja123, in it goes.

maniteja123 · 2014-12-30T12:16:12Z

Thanks @rgommers @jaimefrio @juliantaylor @argriffing, for the help and guidance. It was really good to learn a lot and contribute to numpy :)

rgommers · 2014-12-30T12:45:38Z

@maniteja123 no problem. Keep them coming!

jaimefrio · 2014-12-30T18:09:08Z

One last piece of gith A44D ub wisdom, @maniteja123: I closed issue #5354 manually, but had the commit message included something like fix #5354, it would have been closed automatically. See this to choose your favorite keyword.

maniteja123 · 2014-12-30T22:50:09Z

@jaimefrio, thanks for the advise. It is a nice way of handling issues related to PR. I would take care to follow the convention next time :-)

rgommers added component: numpy._core 00 - Bug labels Dec 22, 2014

maniteja123 force-pushed the issue5354 branch from cf4a998 to 33e1f1b Compare December 22, 2014 18:33

maniteja123 changed the title ~~BUG: Issue 5354 - commented line 3601-02 in numpy/core/src/multiarray/arraytypes.c.src~~ BUG: Issue 5354 - deleted line 3601-02 in numpy/core/src/multiarray/arraytypes.c.src Dec 22, 2014

maniteja123 mentioned this pull request Dec 22, 2014

BUG: Issue 5354 - commented line 3601-02 in numpy/core/src/multiarray/arraytypes.c.src #5385

Closed

rgommers reviewed Dec 22, 2014
View reviewed changes

maniteja123 changed the title ~~BUG: Issue 5354 - deleted line 3601-02 in numpy/core/src/multiarray/arraytypes.c.src~~ BUG: Issue 5354 - Fixed segmentation fault when clipping complex arrays Dec 24, 2014

rgommers reviewed Dec 28, 2014
View reviewed changes

maniteja123 force-pushed the issue5354 branch 2 times, most recently from 0e605e9 to ffb8a1b Compare December 30, 2014 05:25

jaimefrio reviewed Dec 30, 2014
View reviewed changes

BUG: solves complex array clip in issue numpy#5354 and added symmetri…

61f3b10

…c regression test

maniteja123 force-pushed the issue5354 branch from ffb8a1b to 61f3b10 Compare December 30, 2014 10:46

jaimefrio added a commit that referenced this pull request Dec 30, 2014

Merge pull request #5386 from maniteja123/issue5354

fb898ce

BUG: Issue 5354 - Fixed segmentation fault when clipping complex arrays

jaimefrio merged commit fb898ce into numpy:master Dec 30, 2014

jaimefrio mentioned this pull request Dec 30, 2014

clipping complex array crashes python #5354

Closed

maniteja123 deleted the issue5354 branch January 16, 2015 19:17

Uh oh!

BUG: Issue 5354 - Fixed segmentation fault when clipping complex arrays #5386

BUG: Issue 5354 - Fixed segmentation fault when clipping complex arrays #5386

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FAIL: test_clip_complex (test_numeric.TestClip)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!