Incorrect Implementation for Accumulating Batch Gradients in Trainer

[lightning-ai]

Incorrect Implementation for Accumulating Batch Gradients in Trainer

[williamFalcon]

@AS-researcher6 merged to master. should be correct now. pls verify

[AS-researcher6]

All good. Thanks for the fixes!
Working on a SLURM cluster myself so may submit pull requests for expanded Trainer functionality and bring up more things in the future.

[williamFalcon]

@AS-researcher6 good find, i see the bug. the division is done once the step is applied, but the division should be on line 926.

Current order:

1. clip
2. step
3. zero_grad
4. divide accumulated loss by nb accumulated batches

Correct order:

1. divide accumulated loss by nb accumulated batches
2. clip
3. step
4. zero_grad

Submitting the change.
Mind sanity checking PR #88

[AS-researcher6]

Gladly! Not sure if you're done fixing things but I don't see the commit where the loss is divided by self.accumulate_grad_batches before the loss.backward(). Otherwise the original version where self.batch_loss_value += loss.item() is good as long as the self.batch_loss_value is not averaged after.

[williamFalcon]

look at the PR #88
line 928

[AS-researcher6]

Sorry, I don't think it's quite right yet. Accounting for multiple batches needs to happen in the actual loss itself before backpropagation. Otherwise, its like N accumulated additions of loss.backward() rather than N accumulated additions of (1/N * loss).backward()
After line 898 and before loss.backward() on the 903 if statement, there needs to be a line that's loss = loss / self.accumulate_grad_batches. Line 922 should still be self.batch_loss_value += loss.item(). Line 928 can be removed entirely, since the averaging has already been accounted for.

[williamFalcon]

@AS-researcher6 i see. updated. Was following the wrong approach before.

How about now?