Bug fix for repitching & tempo automix errors

Ma5onic commented 5 months ago

Fix for repitching TypeError: unsupported format string passed to numpy.ndarray.__format__ error:

Traceback (most recent call last):
  File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/home/ubuntu/demucs/tools/automix.py", line 343, in <module>
    main()
  File "/home/ubuntu/demucs/tools/automix.py", line 327, in main
    track, origs = build_track(index, catalog)
  File "/home/ubuntu/demucs/tools/automix.py", line 245, in build_track
    wav, spec = get_part(spec, src, dt, dp)
  File "/home/ubuntu/demucs/tools/automix.py", line 209, in get_part
    wav = repitch(wav, dp, dt * 100, samplerate=SR, voice=source == 3)
  File "/home/ubuntu/demucs/demucs/repitch.py", line 74, in repitch
    f"-tempo={tempo:.6f}",
TypeError: unsupported format string passed to numpy.ndarray.__format__

and fix for AttributeError: module 'scipy.signal' has no attribute 'hann' :

concurrent.futures.process._RemoteTraceback:
"""
Traceback (most recent call last):
  File "/usr/lib/python3.10/concurrent/futures/process.py", line 246, in _process_worker
    r = call_item.fn(*call_item.args, **call_item.kwargs)
  File "/home/ubuntu/demucs/tools/automix.py", line 81, in analyse_track
    tempo, events = beat_track(y=drums.numpy(), units='time', sr=SR)
  File "/home/ubuntu/demucs/.venv/lib/python3.10/site-packages/librosa/beat.py", line 185, in beat_track
    beats = __beat_tracker(onset_envelope, bpm, float(sr) / hop_length, tightness, trim)
  File "/home/ubuntu/demucs/.venv/lib/python3.10/site-packages/librosa/beat.py", line 433, in __beat_tracker
    beats = __trim_beats(localscore, beats, trim)
  File "/home/ubuntu/demucs/.venv/lib/python3.10/site-packages/librosa/beat.py", line 507, in __trim_beats
    smooth_boe = scipy.signal.convolve(localscore[beats], scipy.signal.hann(5), "same")
AttributeError: module 'scipy.signal' has no attribute 'hann'
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/home/ubuntu/demucs/tools/automix.py", line 343, in <module>
    main()
  File "/home/ubuntu/demucs/tools/automix.py", line 312, in main
    spec, track = pending.result()
  File "/usr/lib/python3.10/concurrent/futures/_base.py", line 458, in result
    return self.__get_result()
  File "/usr/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result
    raise self._exception
AttributeError: module 'scipy.signal' has no attribute 'hann'

Explanation of code changes:

Conditional Application: The check (dt != 0 or dp != 0) ensures that repitch is only called if there is a change to be made, saving computational resources if the audio is to remain unaltered.

Type and Size Validation for dt: It ensures that dt is a scalar by checking its type and size, which is crucial for correct format string usage in the repitch function's command construction.

Percentage Conversion: The tempo_percentage_change calculation transforms dt from a decimal representation (e.g., -0.12 for a tempo slowing to 88% of the original) into the format expected by the repitch function (e.g., -12 to represent the same).

Application of Changes: Calls repitch with the correct parameters, including checking if the current track is the voice track (assumed to be at index 3).

Onset Adjustment: Adjusts the onsets of the spec to account for the new tempo. The onsets must be scaled by the reciprocal of (1 + dt) because a negative dt implies a reduction in speed (and hence longer duration between onsets).

Ma5onic commented 5 months ago

@CarlGao4 Thanks for the feedback. Sorry I missed the part in the readme about adefossezs' fork. I was just following the CONTRIBUTING.md which states:

Demucs is the implementation of a research paper. Therefore, we do not plan on accepting many pull requests for new features. We certainly welcome them for bug fixes.

This is a bug fix and not a pull request for a new feature, which is why I opened this PR.

As for the requirements.txt, could you explain why I shouldn't be placing librosa in it when the "Requirements" section of README.md states:

See requirements_minimal.txt for requirements for separation only, and environment-[cpu|cuda].yml (or requirements.txt) if you want to train a new model.

This can be confirmed by inspecting the setup.py used by pip, which shows that the that the requirements_minimal.txt is used by default and the requirements.txt is only used if you specify [dev] during the installation (either with pip install -e .[dev] or pip install demucs[dev]) as seen in lines 59 to 62:

    extras_require={
        'dev': ALL_REQUIRED,
    },
    install_requires=REQUIRED,

where:

REQUIRED = load_requirements('requirements_minimal.txt')
ALL_REQUIRED = load_requirements('requirements.txt')

CarlGao4 commented 5 months ago

Sorry that's my mistake. But did you add this requirement to environment-cuda.yml?

Ma5onic commented 5 months ago

@CarlGao4, No problem... It happens. 😉

But did you add this requirement to environment-cuda.yml?

No, not yet... Short Answer: I'm waiting for release 0.10.2 of librosa to be distributed by pip

Long Answer:

The last step in the instructions for machine learning scientists is to install requirements.txt
Currently, the latest versions of librosa available via pip or conda-forge is 0.10.1, which is the version that was causing the bug. When trying to locate librosa/beat.py", line 507 from the Traceback output in the master branch, I noticed that smooth_boe = scipy.signal.convolve(localscore[beats], scipy.signal.hann(5), "same") no longer existed at its original location.
Once I located where smooth_boe is currently being defined, I noticed that it now used numpy instead of scipy.signal. Since the new smooth_boe implementation is what is being used in the planned 0.10.2 release, I decided to install from source instead of writing a fix that would break once version 0.10.2 of librosa gets officially released.
When using cloud GPU instances for training, not all of them have conda pre-installed. Setting librosa in requirements.txt allows it to be used using venv + pip as well.
The conda dependency resolver can sometimes take ridiculous amounts of time & still fail to resolve dependencies, which can be costly when using a cloud GPU instance.

However, I do agree that once version 0.10.2 of librosa gets released to the pip package manager (and/or conda), it should also be specified in the - pip: section of environment-cuda.yml & environment-cpu.yml. Technically, the values of the - pip: requirements within the environment-*.yml should match what is in requirements.txt (and vice versa), which is a separate issue that I will fix in a later pull request/commit.

JuYangFu commented 4 months ago

Hi, why do I still have this problem after I modified it the way you did?

CarlGao4 commented 4 months ago

Maybe you should upgrade Python and numpy? (Like Python 3.9 and numpy 1.25.2 which is distributed with PyPI, not conda)

facebookresearch / demucs

Bug fix for repitching & tempo automix errors #593

Explanation of code changes: