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: add input validation to scales argument to cwt #703

Merged
merged 6 commits into from
Mar 8, 2024

Conversation

cyschneck
Copy link
Contributor

@cyschneck cyschneck commented Feb 23, 2024

Overview

Warning for when using invalid scale range that includes zero which is easy to accidentally do when setting up scales dynamically and would be nice to fail gracefully.

Bug and Steps to Reproduce

When running an invalid scale range (that includes zero), pywt throws an unrelated IndexError

time, sst_data = pywt.data.nino()
scales = [0, 1, 2]
wavelet_coeffs, freqs = pywt.cwt(data=sst_data,
                                 scales=scales,
                                 wavelet="morl")

Throws:

pywt/_cwt.py:154, in cwt(data, scales, wavelet, sampling_period, method, axis)
    152 if j[-1] >= int_psi.size:
    153     j = np.extract(j < int_psi.size, j)
--> 154 int_psi_scale = int_psi[j][::-1]
    156 if method == 'conv':
    157     if data.ndim == 1:

IndexError: index -9223372036854775808 is out of bounds for axis 0 with size 1024

This error actually originates on pywt/_cwt.py:150 when attempting to divide by the scale. If the scale range includes zero then it will attempt to divide by zero. However, this error is hidden by casting with astype.

For example, the example below should throw a divide by zero issue, but doesn't. It instead returns an empty array. As a result, trying to index by the empty array on line 154 will throw IndexError: index -9223372036854775808 is out of bounds for axis 0 with size 1024

For example:

j = np.arange(0 * (28-0)) / (0 * 1)
j = j.astype(int)
print(j[::-1])
--> Returns: []

Solution

Throw relevant ValueError when attempting to use a scale that includes zero

@cyschneck cyschneck changed the title Index error IndexError on Scales with Zero Feb 23, 2024
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks for the issue report and PR @cyschneck. This looks close to right, however it'd be more robust to check for negative values to. scales should contain only positive numbers. So I suggest to:

  • Use if np.any(scales <= 0): as the criterion
  • Also test with an input that has a negative scale value

And while we're at it, this check on lines 118-119 doesn't look very robust:

if np.isscalar(scales):
    scales = np.array([scales])

Can you please replace that with an unconditional:

scales = np.asarray(scales)

?

@cyschneck
Copy link
Contributor Author

With the unconditional conversion for scales

scales = np.asarray(scales)

I added a ValueError if the scale was not given as a array_like (list/np.array), which enforces the array_like expected unit type from the docs

# convert array_like scales to a np.array
if not isinstance(scales, list) and not isinstance(scales, np.ndarray):
    raise ValueError("scales should be an array_like, list or np.array")
scales = np.asarray(scales)

@rgommers rgommers added this to the v1.6.0 milestone Mar 8, 2024
@rgommers
Copy link
Member

rgommers commented Mar 8, 2024

I added a ValueError if the scale was not given as a array_like (list/np.array), which enforces the array_like expected unit type from the docs

Note that array-like includes scalars (and anything else that np.asarray can convert to a numpy array), so this last change is not correct and would be a backwards-compatibility break.

The problem from the second commit was:

FAILED ..\tests\test_cwt_wavelets.py::test_cwt_small_scales - TypeError: iteration over a 0-d array
FAILED ..\tests\test_matlab_compatibility_cwt.py::test_accuracy_precomputed_cwt - TypeError: iteration over a 0-d array

So probably the right solution is to use np.atleast_1d(scales).

@rgommers
Copy link
Member

rgommers commented Mar 8, 2024

I pushed a rebase with that suggestion - let's see if we can get this merged:)

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Okay, CI is happy and this should be robust. Let's give it a go - thanks @cyschneck!

@rgommers rgommers changed the title IndexError on Scales with Zero MAINT: add input validation to scales argument to cwt Mar 8, 2024
@rgommers rgommers merged commit ec9338f into PyWavelets:master Mar 8, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants