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.

Sign up for GitHub

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

Jump to bottom

Recipe for X-UMX (using MUSDB18) #490

Merged
merged 7 commits into from
May 6, 2021
Merged

Recipe for X-UMX (using MUSDB18) #490

merged 7 commits into from
May 6, 2021

Conversation

r-sawata
Copy link
Contributor

@r-sawata r-sawata commented May 5, 2021 •
edited
Loading

Thank @TE-StefanUhlich, @faroit and @mpariente for lots of efforts!! I would like to do PR regarding X-UMX.

The differences between official master and xumx_branch are summarized as follows:
[newly added]
･asteroid/tests/models/xumx_test.py
･asteroid/egs/musdb18/X-UMX
･asteroid/asteroid/models/x_umx.py
[modified]
･asteroid/asteroid/models/__init__.py
･asteroid/asteroid/utils/hub_utils.py

@r-sawata r-sawata changed the title [Add] X-UMX for music source separation (MUSDB18) Recipe for X-UMX (using MUSDB18) May 5, 2021
@mpariente
Copy link
Collaborator

Thanks @r-sawata !

Just for the background, this is going to be one of the baselines of the Music demixing challenge and was subject to heavy review in a private fork by @faroit, @TE-StefanUhlich and me.

I'll make a last review tomorrow.

mpariente
mpariente approved these changes May 6, 2021
View reviewed changes
Copy link
Collaborator

@mpariente mpariente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made few changes, and asked a few small others.
When those are addressed, we can merge it.

egs/musdb18/X-UMX/local/conf.yml Outdated Show resolved Hide resolved
tests/models/xumx_test.py Outdated Show resolved Hide resolved
egs/musdb18/X-UMX/train.py Show resolved Hide resolved
tests/models/xumx_test.py Outdated Show resolved Hide resolved
mpariente
mpariente reviewed May 6, 2021
View reviewed changes
Copy link
Collaborator

@mpariente mpariente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm about the requirements installation that it's ok for you please?
We can merge afterwards

@r-sawata @mpariente
Update egs/musdb18/X-UMX/run.sh
8fdfd8e 
Co-authored-by: Pariente Manuel <[email protected]>
@r-sawata
Copy link
Contributor Author

r-sawata commented May 6, 2021

Thank you for your revising, @mpariente! I'm fine for all of your suggestions.

@mpariente mpariente merged commit f1907be into asteroid-team:master May 6, 2021
@mpariente
Copy link
Collaborator

Merged! Thanks to all involved !

@r-sawata
Copy link
Contributor Author

r-sawata commented May 7, 2021

@mpariente
Sorry! I forgot adding 2 packages: museval and norbert.
Could you add followings to egs/musdb18/X-UMX/requirements.txt in master branch?

museval>=0.4.0
norbert>=0.2.1

I'm sorry for your inconvenient.

@mpariente
Copy link
Collaborator

I let you make a new PR

@r-sawata
Copy link
Contributor Author

r-sawata commented May 7, 2021

OK, I will follow your suggestion. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpariente mpariente mpariente approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@r-sawata @mpariente