Neo tests fail with NumPy 1.15.0's assert_array_equal

[jpgill86]

I'm finding that several tests are failing on Travis CI, depending on the version of NumPy installed on the test server. The problem is isolated to the latest NumPy, version 1.15.0. When the version of NumPy is not forced, it varies depending on the Python version. All of the tests that fail rely on NumPy's assert_array_equal.

I ran several tests to demo my findings. From master, I added stderr print statements so that Travis CI build logs would display the version of NumPy installed (search for "@@@@@" to find them quickly). I then tried building with and without forcing the version of NumPy.

NumPy version NOT forced:

• Commit demoing behavior: 940c449
• Travis CI report: https://travis-ci.com/jpgill86/python-neo/builds/82022219
• Result: Python 2.7, 3.4, 3.6 pass because they install NumPy 1.12.1 (Py 3.4) or 1.13.3 (Py 2.7, 3.6) by default, whereas Python 3.5 fails because it installs NumPy 1.15.0 by default.

NumPy version forced to 1.15.0:

• Commit demoing behavior: dd54a60
• Travis CI report: https://travis-ci.com/jpgill86/python-neo/builds/82022633
• Result: All Python versions fail in the same way

NumPy version forced to 1.14.5:

• Commit demoing behavior: 6f25856
• Travis CI report: https://travis-ci.com/jpgill86/python-neo/builds/82023437
• Result: All Python versions pass without errors

I don't think this was a problem only a few days ago (see latest master branch Travis CI report here). My guess is that in the last few days the default NumPy version for Python 3.5 changed from something older to 1.15.0. I'm not sure who controls that (PyPI? Travis CI?). When this happens for other Python versions, more builds will fail.

[jpgill86]

I just noticed another difference between the failing Python 3.5 build on Travis CI and the others which pass (see first report here). The Python 3.5 build currently uses pip version 18.0, whereas the others use pip version 9.0.1.

This was not the case in the latest master branch report from a week ago, where the report shows that pip version 9.0.1 was used for all builds.

So, within the last week, the version of pip used for Python 3.5 builds on Travis CI changed from 9.0.1 to 18.0, and with it the default choice of NumPy changed from older versions to the latest, 1.15.0, which breaks Neo's tests that use NumPy's assert_array_equal.

Regardless of the cause of the discrepancies in build defaults, the bug needs to be tracked down. I haven't looked at the relevant code, but my guess is that Neo has some code that NumPy's assert_array_equal uses for determining whether two Neo objects, such as AnalogSignals, are the same or not. For whatever reason, this code works correctly with NumPy < 1.15.0 but not with NumPy == 1.15.0.

[jpgill86]

I've tracked this issue further.

(By the way, NumPy 1.15.1 was released yesterday, and the problem persists with that version.)

As of version 1.15.0, NumPy's assert_array_equal calls a special subroutine on its arguments to check for infinities in the arrays. This new subroutine is called func_assert_same_pos.

(This new subroutine is part of a major reorganization/rewrite of test functions that occurred when NumPy's test suite switched from Nose to PyTest.)

When called by assert_array_equal, the subroutine func_assert_same_pos essentially executes x == numpy.inf and y == numpy.inf, where x and y are the arguments to assert_array_equal. What's going on here is that an array is being compared element-wise to a scalar, and the expected output is an array of booleans that indicate where the comparison is true or false.

From what I can tell, assert_array_equal is designed to work on numpy.ndarray objects generically. Subclasses of ndarray are expected to properly implement comparisons of the array to scalars, which should result in an array of booleans. I think that maybe this has always been the case (i.e., it's not a new constraint added in 1.15.0). It's just that, in some cases, subclasses of ndarray got by without meeting this expectation in the past.

neo.AnalogSignal (actually neo.BaseSignal) inherits from numpy.ndarray (via quantities.Quantity), but it does not meet this expectation. The use of == on an AnalogSignal invokes its __eq__ method, which assumes the two things being compared are both AnalogSignals because it first compares the arguments' t_start values. If an AnalogSignal is compared to a scalar, the error that is the cause of this issue arises.

Here's a simple example that does not depend on the version of NumPy and which causes this error:

import neo
import numpy as np
import quantities as pq

sig = neo.AnalogSignal([0.1, 0.1, 0.1], sampling_rate=1*pq.Hz, units=pq.uA)

sig == 0.1

Here is the error:

Traceback (most recent call last):
  File "neo_array_equals.py", line 7, in <module>
    sig == 0.1
  File "c:\users\jeffrey\documents\python-dev\neo\git-repo\python-neo\neo\core\analogsignal.py", line 416, in __eq__
    if (self.t_start != other.t_start or
AttributeError: 'float' object has no attribute 't_start'

I think that the solution to this issue is to change __eq__ to first check the type of the two objects being compared, so that it can handle comparisons of AnalogSignals both to scalars and to other AnalogSignals. I'll take a crack at implementing this.

[jpgill86]

Take a look at #557.

Since this issue relates to the use of == between signals and scalars, I first added some tests for those comparisons (2bebe4d), which of course failed initially. While I was at it, I also added tests for the use of == between two signals.

In my second commit (16340f0), I attempted to fix the problem by adding checks within the __eq__ methods of AnalogSignal and IrregularlySampledSignal for the types of the objects being compared. Time parameters (e.g., t_start) are only checked for equality if the types are compatible. These changes allow my new tests to pass, but only in NumPy < 1.15.0. New problems with version 1.15.0 were unmasked by this change (see Travis CI report here).

Aside: The changes in 16340f0 to IrregularlySampledSignal.__eq__() do more than just check type. They will also change the result for a comparison with == between two IrregularlySampledSignals. Comparison between numpy.ndarrays normally returns an array of booleans. In IrregularlySampledSignal.__eq__(), before this change, that array of booleans was being collapsed down to a single True or False through the use of all(), and that was returned instead. Since this result is inconsistent with the result of a similar comparison between two AnalogSignals (which returns an array of booleans as expected), I changed the behavior for IrregularlySampledSignals. My new tests check that the result is an array of booleans, and no other tests were broken by this change.

The new problem with NumPy 1.15.0 that was unmasked by these changes to __eq__ still pertains to assert_array_equal. Later on in the execution of the function, the new 1.15.0 implementation attempts to index into the signals using the boolean arrays obtained via __eq__. Again, I think this is something that ndarrays are supposed to be capable of in general, but this functionality does not exist for Neo signals.

Here's a simple example:

import neo
import numpy as np
import quantities as pq

sig = neo.AnalogSignal([0.1, 0.2, 0.3], sampling_rate=1*pq.Hz, units=pq.uA)

# this works after the fix in 16340f0
print(sig == 0.2)
# result: [[False]
#          [ True]
#          [False]]

# this does not work and raises an IndexError, and
# this functionality is needed for assert_array_equal in NumPy >= 1.15.0
print(sig[sig == 0.2])

The output:

[[False]
 [ True]
 [False]]
Traceback (most recent call last):
  File "neo_array_equals.py", line 15, in <module>
    print(sig[sig == 0.2])
  File "c:\users\jeffrey\documents\python-dev\neo\git-repo\python-neo\neo\core\analogsignal.py", line 308, in __getitem__
    raise IndexError("index should be an integer, tuple or slice")
IndexError: index should be an integer, tuple or slice

The code that would need to change to accommodate the new requirements of assert_array_equal would be located in AnalogSignal.__getitem__() and IrregularlySampledSignal.__getitem__(). As is already stated in a comment, arbitrary indexing into a signal would probably require that an IrregularlySampledSignal be returned.

Since this is a lot of work, I'm going to stop working on this and wait to hear from others about whether this is the right direction to take. An alternative solution might be to stop using assert_array_equal entirely and instead implement Neo's own assertion function for signal comparison. This could have its own problems.

[JuliaSprenger]

Thanks for digging into this @jpgill86.
On a first glance I think returning an IrregularlySampledSignal for arbitrary indexing makes sense and might be simpler than implementing a custom assert_array_equal.
However, there is also a discussion related to the SpikeTrain.__getitem__() #490, which is still open. There the discussion is about whether to return a neo object or a scalar when accessing a single index of a SpikeTrain. Currently the general tendency is to return scalars in this case. It would be good to have a consistent solution for both cases. @bjoern1001001, @samuelgarcia What is your opinion on that?

[jpgill86]

Another possible return type for AnalogSignal.__getitem__() and IrregularlySampledSignal.__getitem__() when non-contiguous selections are made is Quantity arrays. This could be simpler than returning an IrregularlySampledSignal, though it discards all temporal information.

[apdavison]

Thanks @jpgill86 for the excellent detective work.