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

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

Open
haberdashPI opened this issue Dec 12, 2019 · 12 comments
Open

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

haberdashPI opened this issue Dec 12, 2019 · 12 comments

Comments

@haberdashPI
Copy link
Contributor

haberdashPI commented Dec 12, 2019

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
Copy link
Collaborator

ssfrr commented Dec 16, 2019

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
Copy link
Contributor Author

haberdashPI commented Jan 9, 2020

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.

  • For frames, I define it as its own unit: we discussed this in my PR for SampledSignals to make use of Unitful, and at the time we decided to use your idea of defining it as a dimensionless quantity. However, I have run into some issues with getting this to work properly for all the types of inputs (e.g. infinite lengths) I wanted to use for inframes.
  • For mix: I want to argue that this is best placed in SignalOperators; the function there handles the same cases as mix here, and then some (e.g. it resamples automatically, and it zero pads as necessary). Given that mix isn't even documented, that I know of, within SampledSignals, I'm guessing it isn't often used. What are your thoughts?

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
Copy link
Collaborator

ssfrr commented Jan 19, 2020

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
Copy link
Contributor Author

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
Copy link

mchitre commented Mar 18, 2020

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
Copy link

mchitre commented Mar 18, 2020

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
Copy link
Collaborator

ssfrr commented Mar 18, 2020

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
Copy link
Contributor Author

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
Copy link

mchitre commented Mar 18, 2020

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
Copy link
Contributor Author

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
Copy link

mchitre commented Mar 20, 2020

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

@ssfrr
Copy link
Collaborator

ssfrr commented Mar 20, 2020

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.

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

3 participants