Skip to content

fix(data): actually honour bounds_check - #45

Open
AnikethCheluva wants to merge 1 commit into
graph-hpt-flowfrom
graph-bounds-gate
Open

fix(data): actually honour bounds_check#45
AnikethCheluva wants to merge 1 commit into
graph-hpt-flowfrom
graph-bounds-gate

Conversation

@AnikethCheluva

Copy link
Copy Markdown

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

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>
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.

1 participant