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

Support SignalBase #5

Closed
mchitre opened this issue Nov 29, 2021 · 13 comments
Closed

Support SignalBase #5

mchitre opened this issue Nov 29, 2021 · 13 comments

Comments

@mchitre
Copy link

mchitre commented Nov 29, 2021

May I recommend that you support https://github.com/haberdashPI/SignalBase.jl (almost the same API as SampledSignals.jl -- and someday SampledSignal.jl is meant to migrate to it -- see JuliaAudio/SampledSignals.jl#56)? That way you'll also end up adding support for other implementations of SignalBase (e.g. https://github.com/haberdashPI/SignalOperators.jl, https://github.com/org-arl/SignalAnalysis.jl, etc).

@mchitre
Copy link
Author

mchitre commented Nov 29, 2021

Also you might want to consider using https://github.com/JuliaPackaging/Requires.jl to add support for SampledSignal.jl, without bringing it in and all its dependencies as dependencies.

@JeffFessler
Copy link
Owner

Thanks for the suggestion of using Requires. I had heard of it, but I had not taken the time to understand its purpose before and now I get it. I added that to PR #6.

@JeffFessler
Copy link
Owner

JeffFessler commented Nov 29, 2021

I don't understand how to use SignalBase. I don't see any type there, just methods for Array types.
But sound(x::AbstractMatrix) is already a method that uses the default sampling rate so I don't see how to dispatch specifically with something from SignalBase, whereas SampleBuf is a specific type so I can dispatch on it. Please clarify or feel free to make a PR. Thanks!

@mchitre
Copy link
Author

mchitre commented Nov 29, 2021

Fair enough. Slightly tricky.

The way SignalBase works is by defining a framerate(x) on any vector-or-matrix-like type to give the sampling rate. Any type that defines this provides a sampling rate (e.g. SampledSignal in SignalAnalysis.jl) and so all you need is to define a sound(x::AbstractVector, S::Real = framerate(x) method to work with it.

The only tricky bit is that SignalBase currently doesn't provide a fallback method. Instead SignalOperators does (returns missing), and so does SignalAnalysis (returns 1). The simplest solution would be to move the fallbacks to SignalBase, so that you can provide a default if you see S is missing.

@haberdashPI, @ssfrr any thoughts?

@haberdashPI
Copy link

haberdashPI commented Nov 29, 2021

Fallback methods have to be opinionated, and it is not clear what the fallback should be, in general. The fact that SignalAnalysis and SignalOperators use different defaults seems indicative of this problem to me (sometimes 1 makes sense, sometimes missing makes sense).

More generally, SignalBase is intended as a generic, traits based approach to signal definitions. At some point it might make sense to include a Signal trait, but I had been experimentng with several approaches to this in SignalOperators, and hadn't settled on one yet, so I didn't include it in SignalBase (which is meant to be more stable).

To use something that supported Signal Base you would have to use duck typing. I.e. just try the methods on some generic Any value, and hope they work. If you want to constrain it to some subset of types, you'd need use trait based dispatch (e.g. http://ucidatascienceinitiative.github.io/IntroToJulia/Html/DispatchDesigns)

@JeffFessler
Copy link
Owner

Perhaps a fallback could go here then, because 8192 Hz is specific to Matlab's history.

framerate(x::Any) = 8192 # fallback
framerate(x::SampleBuf) = x.samplerate # might be unnecessary eventually?

sound(x::AbstractArray, S::Real = framerate(x)) = ...

Thoughts?

@haberdashPI
Copy link

Just my two cents, and I don't have a horse in this race but I personally feel, given the use here, that a request for a samplerate from a signal without one should be an error of some sort.

framerate(x::Any) = error("The signal you are trying to play as a sound does not have a known samplerate. ",
                          "Consider wrapping in a `SampleBuf` or defining `framerate(x::MyType)`").

@JeffFessler
Copy link
Owner

This discussion has compelled me to jettison the obsolete default of 8192 Hz and instead defer to framerate. Thus will force the caller to either specify the rate as a positional argument or pass a type that has a rate associated with it. So we lose a tiny bit of compatibility with Matlab's outdated default, while gaining generality and safer practices.

Personally I like samplerate better than framerate for audio, but it doesn't really matter to me because my students will always be passing in the rate argument as a Real. It seems that I don't even need any dependence on SignalBase here.

Thanks for the great input and feedback. I think that #7 should close this issue but I'll leave it open for a bit in case anyone sees any issues with that PR.

@mchitre
Copy link
Author

mchitre commented Nov 30, 2021

Agree with defaulting to framerate rather than 8192 Hz. The only reason you may still need the dependency on SignalBase is that framerate needs to be the exported symbol from that module for it to be compatible with it.

@haberdashPI
Copy link

haberdashPI commented Nov 30, 2021 via email

@JeffFessler
Copy link
Owner

Ok I updated #7 to import SignalBase so that framerate hopefully is used properly.

Since you both obviously work a lot with audio signals in Julia, whereas I am new to this aspect, I wonder if you have any insights about how to make the CI run more smoothly. My runtests are passing fine on macOS, but are failing on windows and linux. My guess is that it is because the mac servers in github's cloud have audio devices in them but the linux and windows servers do not (or something like that) so the CI runs fail out somewhere deep inside PortAudio calls because there is no sound car there. Is there a work around you have found for CI of audio in a cross-platform way?

@mchitre
Copy link
Author

mchitre commented Dec 1, 2021

Great!

I don't have much experience doing CI on a sound card code. The way I'd probably approach it would be to stub the call to the sound card (in this case to PortAudio), and check that the correct values are passed in to the call.

@mchitre mchitre closed this as completed Dec 1, 2021
@JeffFessler
Copy link
Owner

Thanks for the suggestion. FYI, I found an answer in the PortAudio test suite: check devices() first to make sure there is actually an audio device in the system. Incorporated in #10.

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