Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAINT: Test against latest NumPy #221

Merged
merged 14 commits into from
Jun 14, 2024
6 changes: 6 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ jobs:
test:

runs-on: ubuntu-latest
continue-on-error: true
strategy:
matrix:
# We test NumPy dev on 3.11
python-version: ['3.8', '3.9', '3.10', '3.11', '3.12']
requires: ['requirements.txt']
include:
Expand All @@ -37,9 +39,13 @@ jobs:
allow-prereleases: true
- name: Install
run: |
set -eo pipefail
python -m pip install --upgrade pip
python -m pip install -r ${{ matrix.requires }}
python -m pip install -r requirements-dev.txt
if [[ "${{ matrix.python-version }}" == "3.11" ]]; then
python -m pip install --only-binary numpy --pre --extra-index-url https://pypi.anaconda.org/scientific-python-nightly-wheels/simple "numpy>=2.1.0.dev0"
fi
python -m pip install .
- name: Lint
run: |
Expand Down
2 changes: 1 addition & 1 deletion nitime/algorithms/event_related.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@


def fir(timeseries, design):
"""
r"""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Takes care of

  nitime/algorithms/event_related.py:13: SyntaxWarning: invalid escape sequence '\h'

Calculate the FIR (finite impulse response) HRF, according to [Burock2000]_

Parameters
Expand Down
6 changes: 2 additions & 4 deletions nitime/algorithms/tests/test_spectral.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,7 @@ def test_mtm_lin_combo():
mtm_cross = tsa.mtm_cross_spectrum(
spec1, spec2, (weights[0], weights[1]), sides=sides
)
npt.assert_(mtm_cross.dtype in np.sctypes['complex'],
'Wrong dtype for crossspectrum')
assert mtm_cross.dtype == np.complex128, 'Wrong dtype for crossspectrum'
npt.assert_(len(mtm_cross) == 51,
'Wrong length for halfband spectrum')
sides = 'twosided'
Expand All @@ -260,8 +259,7 @@ def test_mtm_lin_combo():
mtm_auto = tsa.mtm_cross_spectrum(
spec1, spec1, weights[0], sides=sides
)
npt.assert_(mtm_auto.dtype in np.sctypes['float'],
'Wrong dtype for autospectrum')
assert mtm_auto.dtype == np.float64, 'Wrong dtype for autospectrum'
npt.assert_(len(mtm_auto) == 51,
'Wrong length for halfband spectrum')
sides = 'twosided'
Expand Down
6 changes: 3 additions & 3 deletions nitime/index_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
'tril_indices_from', 'triu_indices', 'triu_indices_from',
]

from numpy.core.numeric import asanyarray, subtract, arange, \
from numpy import asanyarray, subtract, arange, \
greater_equal, multiply, ones, asarray, where

# Need to import numpy for the doctests!
import numpy as np
# Need to import numpy for the doctests!
import numpy as np

def tri(N, M=None, k=0, dtype=float):
"""
Expand Down
20 changes: 12 additions & 8 deletions nitime/tests/test_timeseries.py
Original file line number Diff line number Diff line change
Expand Up @@ -916,16 +916,20 @@ def test_index_int64():
assert repr(b[0]) == repr(b[np.int32(0)])


def test_timearray_math_functions():
@pytest.mark.parametrize('f', ['min', 'max', 'mean', 'ptp', 'sum'])
@pytest.mark.parametrize('tu', ['s', 'ms', 'ps', 'D'])
def test_timearray_math_functions(f, tu):
"Calling TimeArray.min() .max(), mean() should return TimeArrays"
a = np.arange(2, 11)
for f in ['min', 'max', 'mean', 'ptp', 'sum']:
for tu in ['s', 'ms', 'ps', 'D']:
b = ts.TimeArray(a, time_unit=tu)
npt.assert_(getattr(b, f)().__class__ == ts.TimeArray)
npt.assert_(getattr(b, f)().time_unit == b.time_unit)
# comparison with unitless should convert to the TimeArray's units
npt.assert_(getattr(b, f)() == getattr(a, f)())
b = ts.TimeArray(a, time_unit=tu)
if f == "ptp" and ts._NP_2:
want = np.ptp(a)
else:
want = getattr(a, f)()
npt.assert_(getattr(b, f)().__class__ == ts.TimeArray)
npt.assert_(getattr(b, f)().time_unit == b.time_unit)
# comparison with unitless should convert to the TimeArray's units
npt.assert_(getattr(b, f)() == want)


def test_timearray_var_prod():
Expand Down
18 changes: 11 additions & 7 deletions nitime/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,8 @@ def test_detect_lines():
"""
Tests detect_lines utility in the reliable low-SNR scenario.
"""
np.random.seed(0)

N = 1000
fft_pow = int( np.ceil(np.log2(N) + 2) )
NW = 4
Expand Down Expand Up @@ -286,19 +288,21 @@ def test_detect_lines_2dmode():
Test multi-sequence operation
"""

# This seed affects not just the signal we generate below, but then also
# detect_lines->dpss_windows->tridi_inverse_iteration
np.random.seed(0)
Comment on lines +291 to +293
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of relaxing equality to floating-point closeness and also getting readable debug outputs, consider git-applying this patch:

diff --git a/nitime/tests/test_utils.py b/nitime/tests/test_utils.py
index c5f0a81..92daca7 100644
--- a/nitime/tests/test_utils.py
+++ b/nitime/tests/test_utils.py
@@ -299,9 +299,9 @@ def test_detect_lines_2dmode():
 
     npt.assert_(len(lines)==3, 'Detect lines failed multi-sequence mode')
 
-    consistent1 = (lines[0][0] == lines[1][0]).all() and \
-      (lines[1][0] == lines[2][0]).all()
-    consistent2 = (lines[0][1] == lines[1][1]).all() and \
-      (lines[1][1] == lines[2][1]).all()
-
-    npt.assert_(consistent1 and consistent2, 'Inconsistent results')
+    # Frequencies
+    assert np.allclose(lines[0][0], lines[1][0])
+    assert np.allclose(lines[0][0], lines[2][0])
+    # Betas
+    assert np.allclose(lines[0][1], lines[1][1])
+    assert np.allclose(lines[0][1], lines[2][1])

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with npt.assert_allclose because it will have nicer output on failure than assert np.allclose(...) but agreed either is better than what is there now :)


N = 1000

sig = np.cos( 2*np.pi*np.arange(N) * 20./N ) + np.random.randn(N) * .01

sig2d = np.row_stack( (sig, sig, sig) )
sig2d = np.vstack( (sig, sig, sig) )

lines = utils.detect_lines(sig2d, (4, 8), low_bias=True, NFFT=2**12)

npt.assert_(len(lines)==3, 'Detect lines failed multi-sequence mode')

consistent1 = (lines[0][0] == lines[1][0]).all() and \
(lines[1][0] == lines[2][0]).all()
consistent2 = (lines[0][1] == lines[1][1]).all() and \
(lines[1][1] == lines[2][1]).all()

npt.assert_(consistent1 and consistent2, 'Inconsistent results')
npt.assert_allclose(lines[0][0], lines[1][0])
npt.assert_allclose(lines[0][0], lines[2][0])
npt.assert_allclose(lines[0][1], lines[1][1])
npt.assert_allclose(lines[0][1], lines[2][1])
17 changes: 14 additions & 3 deletions nitime/timeseries.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@
# Our own
from nitime import descriptors as desc

try:
_NP_2 = int(np.__version__.split(".")[0]) >= 2
except Exception:
_NP_2 = True

#-----------------------------------------------------------------------------
# Module globals
#-----------------------------------------------------------------------------
Expand Down Expand Up @@ -112,7 +117,9 @@ def __new__(cls, data, time_unit=None, copy=True):
which are SI units of time. Default: 's'

copy : bool, optional
Whether to create this instance by copy of a
Whether to create this instance by copy of a. If False,
a copy will not be forced but might still be required depending
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc update done

on the data array.

Note
----
Expand Down Expand Up @@ -152,7 +159,7 @@ class instance, or an int64 array in the base unit of the module
e_s += 'TimeArray in object, or int64 times, in %s' % base_unit
raise ValueError(e_s)

time = np.array(data, copy=False)
time = np.asarray(data)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes

>           time = np.array(data, copy=False)
E           ValueError: Unable to avoid copy while creating an array as requested.
E           If using `np.array(obj, copy=False)` replace it with `np.asarray(obj)` to allow a copy when needed (no behavior change in NumPy 1.x).
E           For more details, see https://numpy.org/devdocs/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at the context, nitime was attempting to respect the copy=False flag of the calling function. I think it probably makes sense to be as strict as numpy here, and raise the exception. Then the test cases should be updated with new expectations about whether a copy is made or not.

Alternately, I suppose you could update the documentation to say that copy=False is a best-effort attempt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the test cases should be updated with new expectations about whether a copy is made or not.

Unfortunately it's not just the test cases but deeper in the code. The offending line is:

npt.assert_equal(type(t), type(t.during(e2)))

so this is calling some internal during method -- it shouldn't raise an error. That means we have to go through the stack:

nitime/timeseries.py:1298: in during
    data = np.array([self.data[..., self.time.slice_during(ep)]
nitime/timeseries.py:834: in slice_during
    i_start = self.index_at(e.start)
nitime/timeseries.py:1417: in start
    return TimeArray(self.data['start'],

And decide if start for example shouldn't have this:

    @property
    def start(self):
        return TimeArray(self.data['start'],
                         time_unit=self.time_unit,
                         copy=False)

but instead copy=True. If I change that (and stop) then we get a different error:

        # one millisecond epoch
        e1ms = ts.Epochs(0, 1, time_unit='ms')
        e09ms = ts.Epochs(0.1, 1, time_unit='ms')
        msg = "Seems like a problem with copy=False in TimeArray constructor."
>       npt.assert_equal(e1ms.duration, ts.TimeArray(1, time_unit='ms'), msg)

nitime/tests/test_timeseries.py:617: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/Applications/MNE-Python/1.7.0_0/.mne-python/lib/python3.12/site-packages/numpy/_utils/__init__.py:85: in wrapper
    return fun(*args, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

args = (<built-in function eq>, 1000000000.0 ms, 1.0 ms)
kwds = {'err_msg': 'Seems like a problem with copy=False in TimeArray constructor.', 'header': 'Arrays are not equal', 'strict': False, 'verbose': True}

    @wraps(func)
    def inner(*args, **kwds):
        with self._recreate_cm():
>           return func(*args, **kwds)
E           AssertionError: 
E           Arrays are not equal
E           Seems like a problem with copy=False in TimeArray constructor.
E           Mismatched elements: 1 / 1 (100%)
E           Max absolute difference among violations: 999999999000000000
E           Max relative difference among violations: 9.99999999e+08
E            ACTUAL: TimeArray(1000000000000000000)
E            DESIRED: TimeArray(1000000000)

So it seems like documenting copy=False is an easier, simpler, and more backward-compatible path here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arokem is the decider.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am inclined to go with the solution proposed here and add documentation. IIUC, the documentation will say that we are ignoring the request to set copy=False?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather that it's best effort and will silently fall back to copying if no-copy isn't possible.

else:
if isinstance(data, TimeInterface):
time = data.copy()
Expand Down Expand Up @@ -309,7 +316,11 @@ def mean(self, *args, **kwargs):
return ret

def ptp(self, *args, **kwargs):
ret = TimeArray(np.ndarray.ptp(self, *args, **kwargs),
if _NP_2:
ptp = np.ptp
else:
ptp = np.ndarray.ptp
ret = TimeArray(ptp(self, *args, **kwargs),
effigies marked this conversation as resolved.
Show resolved Hide resolved
time_unit=base_unit)
ret.convert_unit(self.time_unit)
return ret
Expand Down
6 changes: 6 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ skip = "pp* cp38-*_aarch64 cp38-musllinux_*"
# don't bother unless someone asks
archs = ["native"]

test-requires = [
"pytest",
"nitime[full]", # Enable all optional behavior
]
test-command = "pytest -rsx --pyargs nitime"

[tool.cibuildwheel.linux]
archs = ["x86_64", "aarch64"]

Expand Down
Loading