Dust Off Fastaudio #121
Dust Off Fastaudio #121
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #121 +/- ##
==========================================
- Coverage 86.95% 86.94% -0.02%
==========================================
Files 13 13
Lines 843 827 -16
Branches 83 115 +32
==========================================
- Hits 733 719 -14
+ Misses 76 75 -1
+ Partials 34 33 -1 ☔ View full report in Codecov by Sentry. |
setup.cfg
Outdated
| torchaudio>=0.7,<0.9 | ||
| librosa==0.8 | ||
| colorednoise>=1.1 | ||
| fastai #==2.3.1 |
There was a problem hiding this comment.
How should we handle versioning? I can snapshot the versions I am currently using and set them equal as we have here if that makes the most sense.
There was a problem hiding this comment.
I think we should always lock the version to something
There was a problem hiding this comment.
Ok, I will update this
There was a problem hiding this comment.
I updated this, but I still don't really like it. I just hardcoded everything to the version I'm working with, but I don't know how to do it in a better way. Any suggestions?
There was a problem hiding this comment.
Testing something else that seems a bit better:
install_requires =
fastai>=2.7.13,<3
torchaudio>=2.1.1,<3
librosa>=0.10.1,<1
matplotlib>=3.3.3,<3.8
etc....
Basically saying any version greater than the one I tested, but not a major version bump
| @@ -197,9 +198,9 @@ def colored_noise(shape, exponent, fmin=0, device=None): | |||
| s_scale = s_scale ** (-exponent / 2.0) | |||
|
|
|||
| # Calculate theoretical output standard deviation from scaling | |||
| w = s_scale[1:].clone() | |||
There was a problem hiding this comment.
clone was stripping sr. is deepcopy an ok option here or do I need to figure out why sr is getting stripped?
| @@ -50,32 +48,6 @@ def tar_extract_at_filename(fname, dest): | |||
| tarfile.open(fname, "r:gz").extractall(dest) | |||
|
|
|||
|
|
|||
| # fix to preserve metadata for subclass tensor in serialization | |||
| # src: https://github.com/fastai/fastai/pull/3383 | |||
| # TODO: remove this when #3383 lands and a new fastai version is created | |||
There was a problem hiding this comment.
This PR went into place so I removed these because self.storage was causing issues
| a2s = AudioToMFCC.from_cfg(AudioConfig.BasicMFCC()) | ||
| sg = a2s(audio) | ||
| sg.show() | ||
| # def test_show_spectrogram(): |
There was a problem hiding this comment.
commented this out due to #120
|
How do we bump the version of fastaudio? |
|
God I've forgotten, I think you just need to update the change log |
| @@ -15,7 +15,7 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
| strategy: | |||
| matrix: | |||
| python-version: [3.6, 3.7, 3.8] | |||
| python-version: [3.8, 3.9, '3.10', 3.11] | |||
There was a problem hiding this comment.
I could do it around all of them if we want. but to answer your specific question, it is because 3.10 is treated as a number and the 0 is chopped off so it tried testing using 3.1 rather than 3.10 if you don't add the quotes. The rest resolve fine as is. I am definitely fine putting a quote around all of them though just for consistency
There was a problem hiding this comment.
I've added quotes to all of these in the updated version. Should I resolve the conversation or should you? I have done it both ways in different repos
I think we may just need to push to master and the github actions will create a new release. The changelog that I see (CHANGELOG.rst) doesn't look like it's been used (unless I misunderstand how it works) |
|
Yeah I think so but can create a new PR to try it if it doesn't work |
The goal if this PR is to get fastaudio back into a working state with a current version of fastai. My main goal was to get all warnings and errors resolved to get things working properly. When going through this, I didn't specify any specific libraries but that is something I think we should probably add back in before this could be merged. Huge thanks to #118 for getting this most of the way. I ended up using quite a bit of the same code and probably ran into a lot of the same issues they did initially.
closes #122 - updates ubuntu-18.04 to ubuntu-latest for one of the github action ymls