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

Improve code readability and make number of epochs a command line argument #1222

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

lancerts
Copy link
Collaborator

@lancerts lancerts commented Jan 24, 2024

Summary

  • Make number of epochs a command line argument, the CI job runs 5 epoch (compared to its default value of 30) to reduce the overall testing time
Screenshot 2024-01-24 at 2 01 36 PM
  • Improve code readability by separating the discriminator creation from the main function.
  • const referece in const torch::data::Example<>& batch : *data_loader is standard (compared with the non-const version torch::data::Example<>& batch : *data_loader).
  • fix warning C4244: 'initializing': conversion from 'double' to 'int64_t', possible loss of data and warning C4244: 'initializing': conversion from 'double' to 'const int64_t', possible loss of data
    with change
    const int64_t batches_per_epoch = static_cast<int64_t>(std::ceil(dataset.size().value() / static_cast<double>(kBatchSize)));

Copy link

netlify bot commented Jan 24, 2024

Deploy Preview for pytorch-examples-preview canceled.

Name Link
🔨 Latest commit a50c1db
🔍 Latest deploy log https://app.netlify.com/sites/pytorch-examples-preview/deploys/65b1ce4ef9152e0008661093

@lancerts lancerts marked this pull request as ready for review January 24, 2024 21:59
int main(int argc, const char* argv[]) {

if (argc > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

a bit ugly no? Isn't there something like argparser in the cpp community?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:P yeah, it is indeed awkward. Adopted the seemingly popular argparser and updated PR.

@msaroufim msaroufim merged commit a537659 into pytorch:main Jan 25, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants