BUG: Identified bug with numpy 1.7 ufunc operators by cpelley · Pull Request #1052 · SciTools/iris

cpelley · 2014-02-28T10:46:30Z

Provides testing framework for iris math operators.
Also provides workaround for bug present in numpy ufunc.
https://groups.google.com/forum/#!topic/scitools-iris/WvGKWUGne00

Numpy issue raised:
numpy/numpy#4425

cpelley · 2014-03-03T13:47:48Z

Ready for review

pelson · 2014-03-04T09:06:14Z

Is there also a numpy PR to fix the underlying issue?

pelson · 2014-03-04T09:08:41Z

I see it is number 4425 on the numpy github tracker.

rhattersley · 2014-03-04T09:09:05Z

Renaming add as op obfuscates what's going on. Especially as op has the same name as self.op.

In fact, there's no real need for the from ... syntax. Just import operator and then later return operator.add would be even more clear.

cpelley · 2014-03-05T18:47:06Z

Thanks both. Made the changes requested, ready for review.

Is there also a numpy PR to fix the underlying issue?

Sry @pelson, I can confirm its numpy/numpy#4425
I have updated the ticket description to reflect this.

cpelley · 2014-03-12T12:29:35Z

bump
I'm tempted to say that this should go out in an emergency tag release..

pp-mo · 2014-03-24T11:53:39Z

Just picking this up, more to come...
But to be going on with

I'm tempted to say that this should go out in an emergency tag release..

I'm inclined to agree.
As the problem produces subtle errors, we should probably target 1.6.x not master.
As discussed, best dealt with here at first, but when agreed let's switch to 1.6.x (and then merge to master also).

pp-mo · 2014-03-24T12:01:20Z

The use of abstract classes deriving from unittest.TestCase can cause problems because unittest (though not nose, I think) will try to run them.
In this case, for example...

itpp@eld238: /net/home/h05/itpp/git/iris/iris_main/lib/iris/tests > python -m unittest iris.tests.unit.analysis.maths  
Traceback (most recent call last):
  File "/usr/local/sci/lib/python2.7/runpy.py", line 162, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/local/sci/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/usr/local/sci/lib/python2.7/unittest/__main__.py", line 12, in <module>
    main(module=None)
  File "/usr/local/sci/lib/python2.7/unittest/main.py", line 94, in __init__
    self.parseArgs(argv)
  File "/usr/local/sci/lib/python2.7/unittest/main.py", line 149, in parseArgs
    self.createTests()
  File "/usr/local/sci/lib/python2.7/unittest/main.py", line 158, in createTests
    self.module)
  File "/usr/local/sci/lib/python2.7/unittest/loader.py", line 130, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/local/sci/lib/python2.7/unittest/loader.py", line 103, in loadTestsFromName
    return self.loadTestsFromModule(obj)
  File "/usr/local/sci/lib/python2.7/unittest/loader.py", line 65, in loadTestsFromModule
    tests.append(self.loadTestsFromTestCase(obj))
  File "/usr/local/sci/lib/python2.7/unittest/loader.py", line 56, in loadTestsFromTestCase
    loaded_suite = self.suiteClass(map(testCaseClass, testCaseNames))
TypeError: Can't instantiate abstract class _TestInplace with abstract methods func
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main/lib/iris/tests >

Problem is discussed here -- http://stackoverflow.com/questions/4566910/abstract-test-case-using-python-unittest

We already have some tests that fix this by using multiple-inheritance
-- for example: https://github.com/SciTools/iris/blob/master/lib/iris/tests/unit/time/test_PartialDateTime.py#L183

pp-mo · 2014-03-24T12:02:02Z

Abstract test classes can cause problems when running via unittest -- see separate comment #1052 (comment)

pp-mo · 2014-03-24T13:09:16Z

Something is wrong here, because when I try to run the tests on the earlier master commit that this branch is based from, I don't get the expected masked data error but I do get a spurious error in a different test (which think I have traced + will explain later...).

Bear with me, this is awkwardly complex to demonstrate, but here goes ...

First check what test results are in the current branch.

itpp@eld238: /net/home/h05/itpp/git/iris/iris_main > git checkout cp_math_mask_bug
Previous HEAD position was 3ac14f3... Merge pull request #1051 from esc24/fixnormlink
Switched to branch 'cp_math_mask_bug'
Purging pyc files and empty directories...
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main > python lib/iris/tests/unit/analysis/maths/test_add.py -v
test_inplace_default (__main__.TestInplace) ... ok
test_inplace_false (__main__.TestInplace) ... ok
test_inplace_true (__main__.TestInplace) ... ok
test_div_partial_mask (__main__.TestValue) ... ok
test_div_zero_masked (__main__.TestValue) ... ok
test_div_zero_unmasked (__main__.TestValue) ... ok

----------------------------------------------------------------------
Ran 6 tests in 0.009s

OK
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main > python -m unittest -v iris.tests.unit.analysis.maths.test_add.TestValue.test_div_partial_mask
test_div_partial_mask (iris.tests.unit.analysis.maths.test_add.TestValue) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.002s

OK
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main >

NOW revert to the master version preceding your changes, but "patch in" your newly added tests ...

itpp@eld238: /net/home/h05/itpp/git/iris/iris_main > git log --oneline -3
7c0b392 MAINT: Changes based on review.
33c972a BUG: Provided workaround for numpy ufunc.
3ac14f3 Merge pull request #1051 from esc24/fixnormlink
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main > git branch cp_math_mask_bug_BASIS 3ac14f3
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main > git checkout cp_math_mask_bug_BASIS
Switched to branch 'cp_math_mask_bug_BASIS'
Purging pyc files and empty directories...
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main > git checkout cp_math_mask_bug lib/iris/tests/unit/analysis/maths
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main >

Sorry for the mess!! (this would be much easier with tests in a separate repo)

NOW run the pasted-in tests again...

itpp@eld238: /net/home/h05/itpp/git/iris/iris_main > python lib/iris/tests/unit/analysis/maths/test_add.py -v                                     
test_inplace_default (__main__.TestInplace) ... ok
test_inplace_false (__main__.TestInplace) ... ok
test_inplace_true (__main__.TestInplace) ... ok
test_div_partial_mask (__main__.TestValue) ... ok
test_div_zero_masked (__main__.TestValue) ... FAIL
test_div_zero_unmasked (__main__.TestValue) ... ok

======================================================================
FAIL: test_div_zero_masked (__main__.TestValue)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/tests/unit/analysis/maths/__init__.py", line 76, in test_div_zero_masked
    self.assertMaskedArrayEqual(com, res)
  File "/home/h05/itpp/git/iris/iris_main/lib/iris/tests/__init__.py", line 365, in assertMaskedArrayEqual
    np.testing.assert_array_equal(a[~a.mask].data, b[~b.mask].data)
  File "/usr/local/sci/lib/python2.7/site-packages/numpy/testing/utils.py", line 719, in assert_array_equal
    verbose=verbose, header='Arrays are not equal')
  File "/usr/local/sci/lib/python2.7/site-packages/numpy/testing/utils.py", line 600, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Arrays are not equal

(shapes (4,), (8,) mismatch)
 x: array([ 2.,  2.,  2.,  2.])
 y: array([ 0,  0,  0,  0,  0,  0,  0, 64], dtype=uint8)

----------------------------------------------------------------------
Ran 6 tests in 0.012s

FAILED (failures=1)
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main >

NOTE IN PARTICULAR
This testcase ought to fail (matches your error case in the numpy bug report)...

itpp@eld238: /net/home/h05/itpp/git/iris/iris_main > python -m unittest -v iris.tests.unit.analysis.maths.test_add.TestValue.test_div_partial_mask
test_div_partial_mask (iris.tests.unit.analysis.maths.test_add.TestValue) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.002s

OK
itpp@eld238: /net/home/h05/itpp/git/iris/iris_main >

ASIDE: The test that did fail is actually the wrong one.
I think I have traced this to a problem with masks
-- it compares an array with "mask=[False, False, False, False]" to one with "mask=False".
-- (but I don't understand why the message is so odd)
-- N.B. my outstanding PR for a MaskedArrayAllClose (#1065) contains a fix for that using "getmaskarray", but I didn't think it was the time to apply it to all the other test routines.

pp-mo · 2014-03-24T13:11:08Z

The presence of the '_div' in the name is misleading.

pp-mo · 2014-03-24T14:26:13Z

I'm thoroughly confused now, still not sure why your tests are not working as advertised.
However, if it's any help, I can confirm that the fix is doing what was intended ...
with new commit :

>>> import iris
>>> a = np.ma.array([1, 2, 3, 4], mask=[True, False, False, True])
>>> b = np.ma.array([1, 2, 3, 4], mask=[False, True, False, True])
>>> t=a+b; print t; print t.data
[-- -- 6 --]
[1 2 6 4]
>>> ca = iris.cube.Cube(a)
>>> cb = iris.cube.Cube(b)
>>> t=(ca + cb); print t; print t.data.data
unknown / (unknown)                 (-- : 4)
[1 2 6 4]
>>>

with old iris : last result is different --

>>> t=(ca + cb); print t; print t.data.data
unknown / (unknown)                 (-- : 4)
[2 4 6 8]

pp-mo · 2014-03-24T14:31:33Z

NOTE: I was going to suggest that the 4 separate copycat tests seem awkward, but I see we have to do it this way to satisfy the test structure rules. I also wondered whether it can all be done in the lower reaches of the code (ideally in maths._math_op_common), but I think I'm satisifed that doesn't work and we do need to modify each of maths.add/subtract/divide/multiply. Shame ! 😞

pp-mo · 2014-03-24T14:31:51Z

Over to you -- get the tests working properly + we'll see what it looks like then.
Ta @cpelley

This effects binary operators (add, subtract, division and multiply) onlt.

cpelley · 2014-03-25T10:40:48Z

Thanks @pp-mo, back to you.

pp-mo · 2014-03-25T12:43:15Z

On checking this against the "old" version again, I found this has "introduced" a bug.
I say "introduced" because really the code was already wrong to be using array[~array.mask], as it does not do the expected when the mask is a pure boolean instead of an array.

I think my preferred solution would be to use full mask arrays if not insisting on exact equality (i.e. "strict=False").
I.E. something like this ...

* strict (bool): If True, perform a complete mask and data array equality check. If False (default), the masks are compared as full arrays and the data array comparison includes only the unmasked elements. """ if strict: np.testing.assert_array_equal(a.mask, b.mask) np.testing.assert_array_equal(a.data, b.data) else: a_mask, b_mask = ma.getmaskarray(a), ma.getmaskarray(b) np.testing.assert_array_equal(a_mask, b_mask) np.testing.assert_array_equal(a[~a_mask].data, b[~b_mask].data)

I tested this and it seems to deliver the goods, i.e. behaviour like ...
between...

ma.array([1,2], mask=False) and

ma.array([1,2], mask=[False, False])

...compare result, strict = True/False --> FAIL / OK

Of course, all of this applies equally to "assertMaskedArrayAlmostEqual" (the very next thing)

cpelley · 2014-03-25T12:47:30Z

I would like to pull away the bug-fixing of the masked array comparison if possible:
https://github.com/SciTools/iris/pull/1074/files

pp-mo · 2014-03-25T12:59:23Z

I would like to pull away the bug-fixing of the masked array comparison if possible:
https://github.com/SciTools/iris/pull/1074/files

Thanks @cpelley - highly commendable !

I was wondering whether to suggest this anyway, but I thought it might be asking too much.
It does feel to me like the changes proposed could use some testing themselves.
Are you sure you want to go this route though? It's bound to make more work ...

rhattersley reviewed Mar 4, 2014
View reviewed changes

pp-mo reviewed Mar 24, 2014
View reviewed changes

pp-mo closed this Mar 24, 2014

pp-mo reopened this Mar 24, 2014

pp-mo reviewed Mar 24, 2014
View reviewed changes

BUG: Provided workaround for numpy ufunc.

663b60c

This effects binary operators (add, subtract, division and multiply) onlt.

Tidy-up

c3b6da0

pp-mo reviewed Mar 25, 2014
View reviewed changes

Uh oh!

Conversation

cpelley commented Feb 28, 2014

Uh oh!

cpelley commented Mar 3, 2014

Uh oh!

pelson commented Mar 4, 2014

Uh oh!

pelson commented Mar 4, 2014

Uh oh!

rhattersley Mar 4, 2014

Choose a reason for hiding this comment

Uh oh!

rhattersley Mar 4, 2014

Choose a reason for hiding this comment

Uh oh!

cpelley commented Mar 5, 2014

Uh oh!

cpelley commented Mar 12, 2014

Uh oh!

pp-mo commented Mar 24, 2014

Uh oh!

pp-mo commented Mar 24, 2014

Uh oh!

pp-mo Mar 24, 2014

Choose a reason for hiding this comment

Uh oh!

pp-mo commented Mar 24, 2014

Uh oh!

pp-mo Mar 24, 2014

Choose a reason for hiding this comment

Uh oh!

pp-mo commented Mar 24, 2014

Uh oh!

pp-mo commented Mar 24, 2014

Uh oh!

pp-mo commented Mar 24, 2014

Uh oh!

cpelley commented Mar 25, 2014

Uh oh!

pp-mo Mar 25, 2014

Choose a reason for hiding this comment

Uh oh!

cpelley commented Mar 25, 2014

Uh oh!

pp-mo commented Mar 25, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants