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

add tag_or_url for download_pretrained_model #361

Merged
merged 4 commits into from
May 17, 2022

Conversation

roholazandie
Copy link
Contributor

@roholazandie roholazandie commented May 12, 2022

I have changed the tag to tag_or_url, now we can download other models not listed in the PRETRAINED_MODEL_LIST to be able to work with ParallelWaveGAN. For now the models should be uploaded to google drive

@roholazandie
Copy link
Contributor Author

This says that imports are not in alphabetical order, do I need to fix it? Sorry, I am very new to pull requests.

Copy link
Owner

@kan-bayashi kan-bayashi left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
Could you reflect my comments?

parallel_wavegan/utils/utils.py Outdated Show resolved Hide resolved
parallel_wavegan/utils/utils.py Show resolved Hide resolved
parallel_wavegan/utils/utils.py Show resolved Hide resolved
Comment on lines 399 to 400
tar.extract(member, f"{download_dir}/{tag_or_url}")
checkpoint_path = find_files(f"{download_dir}/{tag_or_url}", "checkpoint*.pkl")
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe using url as the directory name looks redundant.
Why don't you use tag for tag specification and URL id for URL specification?
Maybe we can define tag for each case e.g., tag=tag_or_url and tag=id_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like your second suggestion, I define a tag for each case, it looks cleaner.

roholazandie and others added 2 commits May 16, 2022 10:44
Co-authored-by: Tomoki Hayashi <hayashi.tomoki@g.sp.m.is.nagoya-u.ac.jp>
parallel_wavegan/utils/utils.py Outdated Show resolved Hide resolved
parallel_wavegan/utils/utils.py Outdated Show resolved Hide resolved
Copy link
Owner

@kan-bayashi kan-bayashi left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you for adding your great feature :)

@kan-bayashi kan-bayashi merged commit a2fc640 into kan-bayashi:master May 17, 2022
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.

2 participants