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

fix(utils): fix epoch variable name in checkpoint save/load functions #519

Merged
merged 1 commit into from
Apr 30, 2023

Conversation

Scorpi
Copy link
Contributor

@Scorpi Scorpi commented Apr 29, 2023

It seems there was a typo/mistake in the checkpoint save/load functions, and the variable was called iteration instead of epoch, and the same typo is present in the save/load log messages, which was confusing to users.

As you can see in code below, we're actually passing current_epoch into save function.

utils.save_checkpoint(
self.net_g,
self.optim_g,
self.learning_rate,
current_epoch,
Path(self.hparams.model_dir)
/ f"G_{total_batch_idx if self.hparams.train.get('ckpt_name_by_step', False) else current_epoch}.pth",
)
utils.save_checkpoint(
self.net_d,
self.optim_d,
self.learning_rate,
current_epoch,
Path(self.hparams.model_dir)
/ f"D_{total_batch_idx if self.hparams.train.get('ckpt_name_by_step', False) else current_epoch}.pth",
)

I fixed that typo, but left it as an iteration in the actual torch data structure to maintain backward compatibility with existing checkpoint files for this model.

@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.62 ⚠️

Comparison is base (ed3240a) 20.37% compared to head (fbf9da3) 19.75%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #519      +/-   ##
==========================================
- Coverage   20.37%   19.75%   -0.62%     
==========================================
  Files          38       39       +1     
  Lines        3318     3376      +58     
  Branches      450      462      +12     
==========================================
- Hits          676      667       -9     
- Misses       2625     2692      +67     
  Partials       17       17              
Impacted Files Coverage Δ
src/so_vits_svc_fork/utils.py 21.79% <0.00%> (ø)

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@34j
Copy link
Collaborator

34j commented Apr 30, 2023

@Scorpi
Copy link
Contributor Author

Scorpi commented Apr 30, 2023

jaywalnut310/vits@2e561ba/train.py#L225-L226 I don't think it's a typo...

Sorry, do you referer to arguments in these save_checkpoint calls?
But the 4th argument that I changed is epoch, and in my PR I changed it from interval to epoch, so now it should match.
image

src/so_vits_svc_fork/utils.py Outdated Show resolved Hide resolved
@Scorpi Scorpi force-pushed the fix/save-load-var-fix branch from f209593 to fbf9da3 Compare April 30, 2023 13:04
@Scorpi Scorpi requested a review from 34j April 30, 2023 13:05
@34j 34j merged commit 0530ea3 into voicepaw:main Apr 30, 2023
@34j
Copy link
Collaborator

34j commented Apr 30, 2023

@allcontributors add Scorpi code

@allcontributors
Copy link
Contributor

@34j

I've put up a pull request to add @Scorpi! 🎉

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.

3 participants