fix(data): actually honour bounds_check - #45
Open
AnikethCheluva wants to merge 1 commit into
Open
Conversation
This was referenced Sep 8, 2026
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Sep 8, 2026
AnikethCheluva
marked this pull request as ready for review
September 8, 2026 05:02
AnikethCheluva
force-pushed
the
graph-bounds-gate
branch
from
September 9, 2026 22:21
51db623 to
e3ec0b0
Compare
AnikethCheluva
force-pushed
the
graph-hpt-flow
branch
from
September 9, 2026 22:21
a2c4ea5 to
9cfa13d
Compare
Nine data configs pass bounds_check, and nothing read it. MultiDataset.__init__ takes **kwargs, so the flag was accepted and discarded, and the per-sample quantile screen in __getitem__ ran unconditionally on every sample -- training and validation alike -- no matter what the config said. That is worse than a no-op switch. A violation does not raise: _check_bounds returns a message and the sample is replaced with another index via get_fallback_idx. So a run whose config said the screen was off was silently substituting samples, and could train on a different distribution than the config described, with only a log line per (episode, key) to show it. Adds bounds_check to __init__ (default True, so nothing changes for configs that never set it) and gates the screen on it. The abc data configs already say false, so they now take effect for both their train and valid datasets. The assignment sits above the state-only early return, and there is a class-level default, because a deploy-mode instance skips the dataset graph entirely and would otherwise reach __getitem__ with the attribute unset. Tests pin the part that matters: with the screen on, an out-of-bounds or NaN sample is substituted; with it off, that same sample is served as read. An in-bounds sample is untouched either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AnikethCheluva
force-pushed
the
graph-hpt-flow
branch
from
September 10, 2026 17:56
9cfa13d to
9cab47f
Compare
AnikethCheluva
force-pushed
the
graph-bounds-gate
branch
from
September 10, 2026 17:56
e3ec0b0 to
9524481
Compare
This was referenced Sep 10, 2026
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.

Nine data configs pass bounds_check, and nothing read it. MultiDataset.init
takes **kwargs, so the flag was accepted and discarded, and the per-sample
quantile screen in getitem ran unconditionally on every sample -- training
and validation alike -- no matter what the config said.
That is worse than a no-op switch. A violation does not raise: _check_bounds
returns a message and the sample is replaced with another index via
get_fallback_idx. So a run whose config said the screen was off was silently
substituting samples, and could train on a different distribution than the
config described, with only a log line per (episode, key) to show it.
Adds bounds_check to init (default True, so nothing changes for configs
that never set it) and gates the screen on it. The abc data configs already say
false, so they now take effect for both their train and valid datasets.
The assignment sits above the state-only early return, and there is a
class-level default, because a deploy-mode instance skips the dataset graph
entirely and would otherwise reach getitem with the attribute unset.
Tests pin the part that matters: with the screen on, an out-of-bounds or NaN
sample is substituted; with it off, that same sample is served as read. An
in-bounds sample is untouched either way.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com