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

Implemented SignalBase API and changed nframes and nchannels to Signa… #61

Closed
wants to merge 2 commits into from

Conversation

mchitre
Copy link

@mchitre mchitre commented Mar 20, 2020

As discussed in #56, I've implemented the SignalBase API for AbstractSampleBuf.

This replaces nframes and nchannels from SampledSignals with the ones from SignalBase, and re-exports them. The SampledSignals.samplerate and SampledSignals.samplerate! APIs are left alone for backward compatibility. Other APIs (SignalBase.framerate and SignalBase.sampletype) are also implemented, but not re-exported, since they were not defined in SampledSignals to begin with.

All tests pass.

@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #61 into master will increase coverage by 1.46%.
The diff coverage is 65.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   71.12%   72.58%   +1.46%     
==========================================
  Files           6        6              
  Lines         374      383       +9     
==========================================
+ Hits          266      278      +12     
+ Misses        108      105       -3
Impacted Files Coverage Δ
src/SampledSignals.jl 100% <ø> (ø) ⬆️
src/WAVDisplay.jl 0% <0%> (ø) ⬆️
src/SignalGen/SinSource.jl 100% <100%> (+6.25%) ⬆️
src/SampleBuf.jl 67.96% <60.97%> (-2.2%) ⬇️
src/SampleStream.jl 91.66% <70%> (+2.52%) ⬆️
src/units.jl 45% <0%> (+10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e17121...8728993. Read the comment docs.

@ssfrr
Copy link
Collaborator

ssfrr commented Mar 20, 2020

I think we should re-export framerate and deprecate samplerate. I’d rather keep things unified

@ssfrr
Copy link
Collaborator

ssfrr commented Mar 20, 2020

@haberdashPI do you think we should add a framerate! function to SignalBase for mutable samples types? The api I have for samplerate! is that it modifies the metadata but doesn’t actually resample. It’s Nice for playing things back faster and slower

@mchitre
Copy link
Author

mchitre commented Mar 20, 2020

Agree on both suggestions. I didn't deprecate samplerate to avoid breaking code that depends on it. For now, export both samplerate and framerate, and remove it in a later release?

@ssfrr
Copy link
Collaborator

ssfrr commented Mar 20, 2020

Yeah, I think for now we export both, with samplerate showing a deprecation warning that points people to framerate. Then in the future we can remove it when we do the massively-breaking release.

@mchitre
Copy link
Author

mchitre commented Mar 20, 2020

Will make the change shortly.

@haberdashPI
Copy link
Contributor

At the moment SignalBase includes just read-only functions. That's possibly a reason to leave out framerate!?? I don't have a strong opinion.

In SignalOperators, things work a little bit differently. You use Resample and ToFramerate to change the framerate: These do not include a ! because the semantics of that package involves transforming lazy representations of a signal; every operator returns a new (lazy) object.

@mchitre
Copy link
Author

mchitre commented Mar 20, 2020

I don't have a strong opinion on it one way or the other. I've currently left framerate! to belong to SampledSignals in this PR.

@mchitre
Copy link
Author

mchitre commented Mar 22, 2020

I've reverted the changes for #60 from this PR for reasons I will discuss in that issue. Better to have this PR be standalone to handle only #56. I can raise a separate PR for #60 later, once I resolve the issues.

@ssfrr
Copy link
Collaborator

ssfrr commented Apr 13, 2020

Sorry for the delay on this - I haven't forgotten about it! I'm 2 weeks from my PhD defense so should have a little more time to start looking at this again after that.

@mchitre
Copy link
Author

mchitre commented Apr 15, 2020

No worries. No rush. Good luck with your defense!!!

@mchitre
Copy link
Author

mchitre commented Oct 15, 2023

I'm guessing this is no longer of any interest. Closing.

@mchitre mchitre closed this Oct 15, 2023
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.

3 participants