JuliaAudio / SampledSignals.jl

Core types for regularly-sampled multichannel signals like Audio, RADAR and Software-Defined Radio
Other
75 stars 25 forks source link

Create a shared package for `samplerate` and `nchannels`?? #56

Open haberdashPI opened 4 years ago

haberdashPI commented 4 years ago

The package I'm working on, SignalOperators, a uses few of the same function names as the SampledSignals package. I'm wondering if we might discuss moving shared functions to some parent package (AudioBase, SignalBase, or a PR to DSP?), and then depending on that shared package. The goal would be to allow people to use both SampledSignals and SignalOperators OR to just use one of them, without running into clashes in the namespace.

The shared functions are samplerate and nchannels.

I use nsamples, and you use nframes. I could see changing the name of my function to nframes, in which case there would be three shared functions. However, for consistency, it seems like, if it is called nframes it should be called framerate, not samplerate. If a sample is defined to include just one channel's value, than the sample rate is twice as fast for a 2-channel signal than a 1-channel signal.

ssfrr commented 4 years ago

Yeah I've been thinking it would be useful to move those definitions to a lightweight SampledBase (or similar) so they can be shared without needing to depend on all of SampledSignals. I just haven't gotten around to it. Feel free to spin one up and I can switch over SampledSignals to depend on it.

Good point on the inconsistency with nframes vs samplerate. TBH I'm not sure what the right direction to resolve that is. I lean a little towards using nframes and framerate, because it's sometimes useful to differentiate between single-channel samples and multi-channel frames. Also then it matches the terms used in video as well, or when using frames of features like MFCC or STFT frames.

haberdashPI commented 4 years ago

Hey, here is a draft of my proposed SignalBase package.

There were a few additional name overlaps, in addition to the ones noted above: framesand mix.

If you're happy with this proposed shared API, I can make a PR to have SampledSignals use it, once I add it the registry, or we can discuss any changes that could be made for us to both be happy. Feel free to make a PR on SignalBase as needed.

~FYI: if you end up looking at SignalOperators, ignore the stable documentation and pay attention to the developer docs as they are quite different.~ (scratch that, stable docs are relevant now).

ssfrr commented 4 years ago

I think that looks great - no objections here. I think that it's good to bias towards keeping SignalBase as small an unoinionated as possible, so having mix in SignalOperators seems like a good plan. I actually think I'd like to strip a bunch of things out of SampledSignals and make it much simpler - I'm definitely not tied to mix, etc.

To be honest in my own code I've been leaning more an more towards using as many built-in data types as possible and not trying too hard to make more convenient wrapper types. I've actually been considering re-working PortAudio.jl and LibSndFile.jl to return regular unwrapped Arrays. Then they would be "low-level" packages that didn't make a lot of assumptions about how they might be used, and folks could more easily write higher-level convenience packages on top of them, or incorporate them into other code without taking on the whole SampledSignals.jl stuff. SignalBase would be useful for that, because then I can still define methods for framerate etc. and not conflict with other higher-level packages.

haberdashPI commented 4 years ago

Great!

And yeah, I see what you mean about fewer specialized types: that could make it easier to separate out functionality common across different contexts.

mchitre commented 4 years ago

Great move to move these out to SignalBase. I'm currently working on a SignalAnalysis package, which has need for functionality that overlaps with both of yours packages, and so this helps. I've been ambivalent about using bare data types, and wrapped ones, but the need to carry sampling rate as metadata with the array keeps coming up now and then. So having a simple wrapped data type that carries sampling rate is certainly nice.

mchitre commented 4 years ago

Would it make sense to have an abstract type or a trait defined in SignalBase, so that one knows whether the API would work on a specific data type or not?

ssfrr commented 4 years ago

An abstract type seems tough, because many, but not all sampled types would also want to be <: AbstractArray. A trait seems like it could make sense though. TBH I've never really gotten my head wrapped around the different ways to implement traits and their pros and cons, so I don't have an opinion on the best way to do it.

Before we go down that route, can you come up with some concrete situations where we'd actually use it? I'm not opposed, but I want to make sure we're not overcomplicating things.

Maybe start an issue over in SignalBase.

haberdashPI commented 4 years ago

I already use a trait for this in SignalOperators. As of now, I think the implementation there is unnecessarily complicated. It is working well there for my purposes but I need to go back and clean things up a bit.

Also, on that note I have yet to make a PR to make SampledSignals support SignalBase; I've been a bit bogged down with some completely unrelated data analysis, but I should be able to get to that in the next 2 weeks or so if someone doesn't do it before me.

mchitre commented 4 years ago

I have a working version of SampleBuf compatibility with SignalBase that I'm using in SignalAnalysis. It shouldn't be hard to make a PR based on that to SampledSignals. So if you haven't already started work on the PR, I am happy to take a stab at it. Up to you.

haberdashPI commented 4 years ago

Sure! If you already have started working on it, it is probably less work overall. (And I can't imagine it is a hard PR to make in any case).

mchitre commented 4 years ago

Cool. I'll try to get it done this weekend.

ssfrr commented 4 years ago

Great, thanks! Things are a little hectic for me but I should be able to review and merge within a couple days if it's not too complicated.