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

sd.rec: dimensions of returned ndarray #232

Closed
fbosio opened this issue Apr 11, 2020 · 7 comments
Closed

sd.rec: dimensions of returned ndarray #232

fbosio opened this issue Apr 11, 2020 · 7 comments

Comments

@fbosio
Copy link

fbosio commented Apr 11, 2020

Hi! I'm not reporting a bug, but suggesting a minor change.

First, I have to say that I use sounddevice a lot in my projects, and I really like it, so thanks for that. The thing is, the rec function of the module returns a numpy.ndarray (when out is None) whose shape[1] equals the given channels, but the recorded data has always two dimensions (ndim is 2). This could be okay, but both scipy.io.wavfile and soundfile read functions return a 1D numpy array when loading a one-channel audio file. So, both

from scipy.io.wavfile read
sample_rate, audio = read('audio_mono.wav')
print(audio.ndim)

and

import soundfile as sf
audio, sample_rate = sf.read('audio_mono.wav')
print(audio.ndim)

show 1. I think that, if one is loading a mono audio, sd.rec should return a 1D array, like those read functions. Besides, the rec function could take an always_2d boolean argument, like the one in the read function of the Bastian Bechtold's soundfile library, to let one decide the ndim of the returned array.

So why I'm asking here, when I can ask both scipy and soundfile developers to change their functions to complain sd.rec? My criterion is simple: those libraries are older, and consequently, there are more people using those than this one for a while. I hope that "sounds" reasonable. Regards.

@mgeier
Copy link
Member

mgeier commented Apr 13, 2020

Thanks for the suggestion!

It isn't a "minor change", though.

This would break a lot of existing code, so there has to be a reeeeeeeeally good reason to make that change.

First, I have to say that I use sounddevice a lot in my projects, and I really like it, so thanks for that.

Thanks, I appreciate that.

If you like it, you should consider giving the project a star!

I think that, if one is loading a mono audio, sd.rec should return a 1D array, like those read functions.

Yeah, I considered this.

But I wanted the returned array to have the same shape as the arrays provided within the callback functions.
For that, it had to be 2D to make any kind of sense.

Besides, the rec function could take an always_2d boolean argument, like the one in the read function of the Bastian Bechtold's soundfile library, to let one decide the ndim of the returned array.

I'm well aware of this function (because I suggested and implemented it: bastibe/python-soundfile#34) and the always_2d argument (again, I did it: bastibe/python-soundfile#36).
Incidentally, I've also changed the default value to the one you seem to prefer: bastibe/python-soundfile#133. You should read the issue description of that, I think you'll agree.

So why I'm asking here, when I can ask both scipy and soundfile developers to change their functions to complain sd.rec?

Well why should sd.rec() behave the same?
It's not a not drop-in replacement for any of the two.

And it already works perfectly together with sf.write():

>>> import sounddevice as sd
>>> import soundfile as sf
>>> fs = 48000
>>> sf.write('delme.wav', sd.rec(100000, channels=1, samplerate=fs, blocking=True), samplerate=fs)

There is no need to specify whether anything is 1D or 2D, isn't there?

My criterion is simple: those libraries are older, and consequently, there are more people using those than this one for a while.

Just that something is older doesn't mean more people are using it.
But let's assume that is the case anyway.

I wouldn't make huge breaking changes to either of the three libraries at this point.

I hope that "sounds" reasonable.

It absolutely does.
Both ways are reasonable, both are totally possible.

The choice depends on priorities, I guess, and to some part on taste.

IMHO, the sf.read() function is the most important part of the whole library, and it was important to me that it is easy to use in an interactive session, so the API is optimized for that.

OTOH, the sd.rec() function is not the most important function in the library.
It's great if it's easy to use, but it isn't as important in this case.
In this case, being totally aware of the options, I chose API consistency over interactive use.
And I think I would do the same thing if I would write it today.

And there is another argument:
sd.rec() is different from sf.read() because it has the channels parameter which could lead to nonsensical situations if it had an always_2d parameter, e.g.:

... = sd.rec(..., channels=2, always_2d=False)

The result would be always 2D and never 1D, which contradicts always_2d=False.
I guess this isn't a huge problem, but it is somewhat awkward.

You might disagree, but you'll have to come up with an extremely good reason for me to change the default.

We can talk about adding an option, though, as long as it isn't a breaking change.

@fbosio
Copy link
Author

fbosio commented Apr 13, 2020

Thank you for use your time to read this! I gave you a star :)

Your words are very reasonable. Some comments.

It isn't a "minor change", though.

This would break a lot of existing code, so there has to be a reeeeeeeeally good reason to make that change.

Yeah, I understand. To be honest, my only reason is this quarantine we are going through, I just came up with this idea and thought "why not...?". Anyway, I don't want to "break a lot of exiting code", maybe I didn't write my suggestion as I meant.

We can talk about adding an option, though, as long as it isn't a breaking change.

Exactly, this is what I meant, the always_2d parameter could be set to True but default, in order to ensure backward compatibility.

And there is another argument:
sd.rec() is different from sf.read() because it has the channels parameter which could lead to nonsensical situations if it had an always_2d parameter, e.g.:
... = sd.rec(..., channels=2, always_2d=False)

Ok, that does look a bit awkward, but I don't know what else could be done to make it look... less awkward?

Well, thanks again for the interest. If this addition ("addition" seems to be a better word than "change", so thanks) is really that hard, I will close this issue. I don't want to make any kind of mess with this module, it works really well just the way it is. Maybe I just should get used to handle mono audio in 2D column matrices instead of 1D vectors, and that's it.

Regards.

@mgeier
Copy link
Member

mgeier commented Apr 15, 2020

We can talk about adding an option, though, as long as it isn't a breaking change.

Exactly, this is what I meant, the always_2d parameter could be set to True but default, in order to ensure backward compatibility.

OK, but would it be really useful then?

You might as well use

sd.rec(...)[:, 0]

... which is shorter than

sd.rec(..., always_2d=False)

Or you could do sd.rec(...).squeeze(), which is a bit longer but at least it sounds funnier.

sd.rec() is different from sf.read() because it has the channels parameter which could lead to nonsensical situations if it had an always_2d parameter, e.g.:
... = sd.rec(..., channels=2, always_2d=False)

Ok, that does look a bit awkward, but I don't know what else could be done to make it look... less awkward?

Well one way would be for the always_2d parameter not to exist in the first place.

I don't know anything else, but if something comes to your mind, let me know!

Well, thanks again for the interest. If this addition ("addition" seems to be a better word than "change", so thanks) is really that hard,

It depends on what you mean by "hard".

It would be super easy to implement, barely an inconvenience.

The question is whether it would be an actual improvement to the API.

And that may well be, at least to some extent, a matter of taste.

I will close this issue.

No need to close this if you still have something to add.

I'm open for changes/additions.
I might give arguments against them.
If you have good counter-arguments, I'm eager to hear them!
If you have alternative suggestions, as well!

I don't want to make any kind of mess with this module, [...]

Don't worry, we're still just talking.
Feel free to propose messy ideas!

Maybe I just should get used to handle mono audio in 2D column matrices instead of 1D vectors, and that's it.

If you only ever use mono signals, there is no need to "upgrade" them to 2D everywhere.

It's just a matter of doing [:, 0] or .squeeze() on your 2D column matrices, I don't think it's such a big deal.

Why are you even using sd.rec() so often in interactive sessions?

And if you use it that often, why don't you just make your own little wrapper function that returns a 1D array?

If you describe your typical workflow, I can probably better understand the problem?

@mgeier
Copy link
Member

mgeier commented Mar 17, 2021

@fbosio Do you have any further comments/suggestions/answers/questions?

If not, I think we can close this issue.

@fbosio
Copy link
Author

fbosio commented Mar 17, 2021

Sorry, I didn't remember to close it the last time!

Edit: actually, the "Close issue" button is not working for me, I wonder why...

@mgeier
Copy link
Member

mgeier commented Mar 18, 2021

actually, the "Close issue" button is not working for me, I wonder why...

That's strange. I thought you should be able to close it because you created it.
Maybe it's just a temporary glitch.

Anyway, I can close it right now ...

@mgeier mgeier closed this as completed Mar 18, 2021
@fbosio
Copy link
Author

fbosio commented Mar 18, 2021

Thanks!

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

No branches or pull requests

2 participants