fix: stop double-advancing the dataset cursor (half the dataset never trained) - #1
Open
golldyck wants to merge 1 commit into
Open
Conversation
run() advances self.dataset_cursor by batch_size after every step, but _pick_batch ALSO added step * batch_size on top of the cursor. Because the cursor already moves each step, the start offset advanced by 2 * batch_size per step: batches began at 0, 2b, 4b, ... and every other slice of the dataset was silently never trained on (the traversal wrapped after covering only half the data). Resume made it worse — the restored cursor and the step term double-counted again. The original trainer used start = (step * batch_size) % len; the resume-state change introduced the per-step cursor increment without dropping the step term. Fix: dataset_cursor alone is the start offset (it advances each step and is restored on resume), so drop the step term and the now-unused parameter. Adds regression tests asserting contiguous coverage for both a fresh run and a resumed cursor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
run()advances the dataset cursor once per step:but
_pick_batchalso addedstep * batch_sizeon top of that same cursor:The cursor already moves each step, so the start offset advances by
2 * batch_sizeper step:Batches begin at
0, 2b, 4b, …. Every other slice of the dataset never gets trained on, and the traversal wraps after covering only half the data. Simulated over a 20-row dataset withbatch_size=2, the rows ever touched are{0,1,4,5,8,9,12,13,16,17}; rows2,3,6,7,…never get picked. Resume makes it worse. The restored cursor and thestepterm double-count again, so a resumed run doesn't continue contiguously either.Root cause
The original trainer used
start = (step * batch_size) % len. The resume-state change added the per-stepdataset_cursor += batch_sizeinrun()but left thestepterm in_pick_batchin place, so the advance now happens twice.Fix
dataset_cursoris the single source of truth for the next offset. It advances each step and is restored from the checkpoint on resume, so_pick_batchshould use it alone:The
stepparameter is now unused, so it's removed and its only caller updated.Tests
Adds two regression tests to
tests/unit/test_trainer.pythat mirror therun()loop (call_pick_batch(), then advance the cursor bybatch_size):test_pick_batch_covers_dataset_contiguously: a fresh run trains rows0,1,2,…with no gaps (fails on the old code with[0,1,4,5,8,9,…]).test_pick_batch_stays_contiguous_after_resume: a cursor restored to 20 continues20,21,22,…(fails on the old code with[20,21,24,25,28,29]).