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

Clearer error messages when write() is called with incorrectly formatted channels #411

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mcclure
Copy link
Contributor

@mcclure mcclure commented Oct 2, 2023

Resolves issue #404

@mcclure
Copy link
Contributor Author

mcclure commented Oct 2, 2023

Tests

Mono/interleaved when stereo expected:

import soundfile
s = soundfile.SoundFile("outtest.wav", mode = 'w', samplerate=48000, channels=2, subtype='PCM_16')
s.write([0,1,2,3])
print(s)

Output

Invalid shape: (4,) (Expected 2 channels, got 1)

Gave data in row-order rather than column-order (or other way around? idk)

import soundfile
import numpy
s = soundfile.SoundFile("outtest.wav", mode = 'w', samplerate=48000, channels=2, subtype='PCM_16')
s.write(numpy.array([[0,1,2,3,4,5], [6,7,8,9,10,11]], dtype=numpy.int16)) # Oops made 5x2 instead of 2x5
print(s)

Output:

ValueError: Invalid shape: (2, 6) (Expected 2 channels, got 6)

Channel count was bad to begin with:

import soundfile
s = soundfile.SoundFile("outtest.wav", mode = 'w', samplerate=48000, channels=4, subtype='PCM_16')
s.write([[0,0,0,0], [2**32,-2**32,2**32,-2**32]])
print(s)

output:

ValueError: Invalid shape: (4,) (Expected 2 channels, got 1)

If it's confusing that parenthesis are used for both the shape and the note afterward, I could change the parenthetical from round to square brackets.

Copy link
Owner

@bastibe bastibe left a comment

Choose a reason for hiding this comment

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

The new error messages look good, that's a great improvement.

But the code logic is incorrect, I believe. It should follow the original logic.

soundfile.py Outdated
array.ndim == 1 and self.channels != 1 or
array.ndim == 2 and array.shape[1] != self.channels):
raise ValueError("Invalid shape: {0!r}".format(array.shape))
if self.channels not in (1,2):
Copy link
Owner

Choose a reason for hiding this comment

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

This seems wrong. Previously, we were checking if the input array had one or two dimensions. Now we're checking whether we have one or two channels. Those are different things, right?

We support arbitrary numbers of channels (depending on the file type), but only one- or two-dimensional data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I misunderstood what the old code was doing but I see the problem now. Will fix

@mcclure
Copy link
Contributor Author

mcclure commented Oct 4, 2023

With corrections, new tests

import soundfile
s = soundfile.SoundFile("outtest.wav", mode = 'w', samplerate=48000, channels=4, subtype='PCM_16')
s.write([[0,0,0,0], [1,1,1,1]])
print(s)

Output: Successfully writes 4-channel file

import soundfile
s = soundfile.SoundFile("outtest.wav", mode = 'w', samplerate=48000, channels=4, subtype='PCM_16')
s.write([[[1,2],[3,4]],[[5,6],[7,8]]])
print(s)

Output: ValueError: Invalid shape: (2, 2, 2) (too many dimensions)

import soundfile
import numpy
s = soundfile.SoundFile("outtest.wav", mode = 'w', samplerate=48000, channels=2, subtype='PCM_16')
q = numpy.array(9, dtype=numpy.int16)
print(q)
print(q.ndim)
s.write(q) # Oops it's a scalar
print(s)

Output:

9
0
[snip]
ValueError: Invalid shape: (1,) (Expected 2 channels, got 1)

If I change channels= to 1, this successfully writes a 1-channel file.

It appears something earlier in the code is converting scalars (0-dimensional arrays) to 1-dimensional arrays. This is an okay behavior, and I recommend leaving the check in _array_io as submitted so the code works standalone.

@bastibe
Copy link
Owner

bastibe commented Oct 6, 2023

Thank you very much! The error messages are indeed much improved!

Is this ready to merge from your end?

@mcclure
Copy link
Contributor Author

mcclure commented Oct 6, 2023

Is this ready to merge from your end?

Yes.

@bastibe
Copy link
Owner

bastibe commented Oct 7, 2023

Wait, why is _soundfile_data updated in this pull request? This breaks all the tests, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants