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

Iterate through all of dataloader each epoch. #65

Merged

Conversation

belsten
Copy link
Collaborator

@belsten belsten commented Apr 16, 2024

Compute epoch energy as the average of the batch energies.

@belsten belsten requested a review from 9q9q April 16, 2024 18:49
@belsten belsten linked an issue Apr 16, 2024 that may be closed by this pull request
@9q9q
Copy link
Collaborator

9q9q commented Apr 16, 2024

Thanks Alex! Just to confirm I understand what the change is: previously, each epoch would just use the next batch from the dataset, and losses were computed per batch. So during training, the model only saw each example once. Change is each epoch uses the entire dataset, and losses are computed for the whole dataset at each epoch.

It makes sense that the old method would finish very fast, because it only processes len(dataset) samples rather than n_epoch*len(dataset) samples. I'm guessing the loss and filters learned were also not as good?

@belsten
Copy link
Collaborator Author

belsten commented May 3, 2024

Almost. The previous incorrect method saw batch_size*n_epoch samples while the new method sees n_epoch*len(dataset) samples (like you said). And yes, the losses returned previously were only over a single batch and now they are over the whole dataset. The previous method would make sense if we called n_epochs n_batch_updates instead but alas we did not.

In my experience, the loss and filters could be fine with the old method, you would just have to make n_epochs large.

Copy link
Collaborator

@9q9q 9q9q left a comment

Choose a reason for hiding this comment

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

Thanks Alex!

@belsten belsten merged commit 0830756 into main May 8, 2024
1 of 2 checks passed
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.

BUG: learn_dictionary uses only one batch update per epoch
2 participants