search

JuliaMusic / MusicProcessing.jl

Music Processing Library in Julia
Other
87 stars 15 forks source link

Removing SampledSignals.jl as Dep && Borrowing Code #23

Closed ashwani-rathee closed 3 years ago

ashwani-rathee commented 3 years ago

Initial work on removing SampledSignals.jl dependency and code is replaced/put directly here from original SampledSignals.jl

ashwani-rathee commented 3 years ago

Good thing that tests fail properly now🤣, we are close to rising from the dark age in this package

ashwani-rathee commented 3 years ago

@jparcill can you confirm if speedup, pitch shift, slowdown worked after your updates?

Since task of current pr is removing sampled signals dependency, I am not sure if I should update speedup and others rn as they hold lot of old, redundant pirated types (means there are functions for stft here but they basically call DSP.Peridograms.stft).

jparcill commented 3 years ago

Yup they work! Not sure how to write proper tests for them but I tested out mp3's and confirmed it by ear

Datseris commented 3 years ago

I say let's merge this and do further updates in different PRs. This PR successfully makes passing tests and removes the SampledSignals.

ashwani-rathee commented 3 years ago

In one of the emails, when jared sent me the ipynb he was using for testing, they do work there. But How do we compares these methods without using duration/size all the time. Librosa does the test of speedup, pitchshift, etc with duration measure only which I am not sure if it tells much about the audio.

Yeah, It would be okay if we merge this one and go to each thing one by one in the following PRs.

rob-luke commented 3 years ago

Hi all,

I see that SampledSignals is no longer a dependency due to this PR. And I understand the benefits of minimising dependencies in your project.

I have just received admin access to the sampled signals repository and will try to reinvigorate it over the coming weeks and months. Probably slowly at first with getting the CIs updated etc, clearing out old PRs, then moving to maintenance mode.

I don’t expect this project to add back SampledSignals as a dependency, but I would be pleased to review any PRs you may push back upstream, and discuss any common issues these two packages may have in common to find optimal solutions together and reduce duplication of our efforts. Similarly I will keep an eye on your great package here.

Datseris commented 3 years ago

Actually, we would be happy to return to have SampleSignals as a dependency here and remove duplicate code. Personally, I'm much in favor of that. We dropped it because we thought it wouldn't be updated anytime soon.

Once you have SS up and running just post a comment here and we can do a simple PR that restores it and removes the code we "stole" from it.

rob-luke commented 3 years ago

Great! I will comment back here and ping you when everything is set. Cheers

rob-luke commented 3 years ago

Sorry I forgot to ping back here @Datseris @jongwook @ashwani-rathee but I think SampleSignals is up and running again, CIs have been stable since https://github.com/JuliaMusic/MusicProcessing.jl/pull/23#issuecomment-835288018 and versions are tagged etc. Please report any issues you have, cheers.

rob-luke commented 3 years ago

FYI my next aim is to steadily increase the test coverage for SampledSignals.