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 validation loss calculation #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alessiamarcolini
Copy link
Contributor

Fixes issue #4

@alessiamarcolini
Copy link
Contributor Author

Hey @sthalles I saw you updated simcrl.py in the last commit, but the behavior of the code is the same as before: counter starts from 0.

Here the problem is not only the ZeroDivisionError (in case you don't have enough validation samples to fill a validation batch - so the for loop never runs), but also that at the end you get a wrong valid_loss.

Let me show you an example with numbers:

  • Say that you have 100 samples in your validation set and a batch size of 20, then you'll perform 5 iterations on the validation set.
  • counter will range from 0 to 4 (included).
  • Suppose that at each iteration you get a loss of 2 (silly example).
  • You're accumulating the losses in valid_loss, so at the end of the 5 iterations valid_loss will be equal to 10.

If at the end you divide valid_loss by counter, you'll get 10/4, resulting 2.5, and not 10/5 resulting 2, as expected.

That's why in this PR I changed the divisor to counter + 1.

Now, I see that simclr.py conflicts with my changes: do you want me to resolve the conflict and push again?

@sthalles
Copy link
Owner

sthalles commented Apr 18, 2020 via email

@alessiamarcolini
Copy link
Contributor Author

Hi Talles,

thank you for your quick response.

Actually I was referring to counter as the index coming from enumerate() function, as in your previous revision, so now it actually works (although using two counter variables instead of one).

Regards

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