Average long_time_metrics time curves over validation batches#93
Open
Adonyth wants to merge 1 commit into
Open
Average long_time_metrics time curves over validation batches#93Adonyth wants to merge 1 commit into
Adonyth wants to merge 1 commit into
Conversation
Inside Trainer.validation_loop, 'time_logs |= new_time_logs' overwrote the long_time_metrics time curves on every batch, so the curves passed to plot_all_time_metrics reflected only the last validation batch. Accumulate them as running means over the batch count instead, exactly mirroring how loss_dict is accumulated a few lines below. Add a lightweight validation_loop test with a stubbed rollout. Fixes PolymathicAI#78
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
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.
Fixes #78
Inside
Trainer.validation_loop, the time curves collected forlong_time_metricsare merged withtime_logs |= new_time_logs, which overwrites every key on each batch — the curves handed toplot_all_time_metricsreflect only the last validation batch. A two-batch toy example makes it concrete: if a metric's curve is 100.0 on batch 1 and 1.0 on batch 2, the logged value is 1.0 instead of the 50.5 average. This is inconsistent withloss_dictthree lines below, which correctly accumulatesloss_dict.get(k, 0.0) + v / denom.The fix mirrors the
loss_dictaccumulation exactly, so the time curves become means over the samedenom(the number of validation batches actually processed):Test note:
Trainerisn't constructible without a full model/datamodule stack, sotests/test_trainer.pybuilds one viaTrainer.__new__, sets only the attributesvalidation_loopreads, stubsrollout_modelto return pre-made(y_pred, y_ref)batches, and captures what reachesplot_all_time_metrics. It asserts the capturedfull_VRMSE_rolloutcurve equals the mean of the two per-batch curves (and not the last batch's curve). The test fails on currentmasterand passes with this change;ruff check/ruff formatare clean under the pre-commit pinned v0.6.4. Happy to restructure the test if you'd prefer a different fixture approach.Found while running an independent quantified-impact study of the benchmark's metric design; happy to adjust to maintainer preferences.