-
Notifications
You must be signed in to change notification settings - Fork 83
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
Conversation
@@ -10,7 +10,7 @@ | |||
|
|||
|
|||
def fir(timeseries, design): | |||
""" | |||
r""" |
There was a problem hiding this comment.
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'
@@ -152,7 +152,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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arokem is the decider.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
nitime/tests/test_utils.py
Outdated
@@ -230,26 +230,27 @@ def test_detect_lines(): | |||
""" | |||
Tests detect_lines utility in the reliable low-SNR scenario. | |||
""" | |||
rng = np.random.RandomState(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@effigies looks like those aarch64
errors were Heisenbugs 🙂
All green, ready for review/merge from my end. Summary:
|
# This seed affects not just the signal we generate below, but then also | ||
# detect_lines->dpss_windows->tridi_inverse_iteration | ||
np.random.seed(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hoping this will fix https://github.com/nipy/nitime/actions/runs/9485767253/job/26138519510 🤞
There was a problem hiding this comment.
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-apply
ing 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])
There was a problem hiding this comment.
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 :)
At a guess, aarch64 pipelining can result in inconsistent ordering or precision extension of FLOPs, so the allclose is necessary. |
All green, ready to go from my end. I guess we just need a decision on #221 (comment) ? |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc update done
Should fail because of
sctypes
but let's see what else there is.In theory this could be a separate run but in practice seems like it should be okay just to tack on to one of the Python version runs for speed.
Closes #218
Closes #219
Closes #221