Preserve bounds for built-in observation aggregators - #562
Open
sylvesterkaczmarek wants to merge 1 commit into
Open
sylvesterkaczmarek wants to merge 1 commit into
sylvesterkaczmarek wants to merge 1 commit into
Conversation
Author
|
@saran-t This observation-layer PR is current with main, has no failing or pending public checks, and has not yet received human review. It preserves BoundedArray bounds through the built-in observation aggregators and includes focused bounds regressions. Could you review the current head when convenient? |
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.
Summary
Fixes #560.
Give built-in observation reducers the
preserves_boundsattribute already consumed and documented byUpdater.observation_spec(). The misspelled attribute made all built-in reducers appear to have unknown bound behavior, so min, max, mean and median discarded BoundedArray limits.These four reducers now retain bounds; sum still explicitly discards them. Reduced values, shapes, dtype promotion, unbounded inputs and custom-aggregator handling remain unchanged. No dynamics, rewards, dependencies or workflows change. This is independent of the initial-padding snapshot fix.
Validation
Added 32 cases covering each reducer's declaration, scalar/per-element bounds, integer/float dtype promotion, rejection of values outside retained limits, unbounded sum results, custom true/false/missing declarations, unbounded inputs and unchanged source arrays. An actual native MuJoCo observation sequence exercises mean aggregation through the Updater.
git diff --check: passed.Based on main at
a04e3e4cf56c12117d2294bb090f9acec21e5c67. Tested on macOS with MuJoCo 3.14.0, NumPy 2.3.2 and SciPy 1.16.1; bindings were generated using build_mjbindings. Rendering was disabled. A broader run failed the same three camera-rendering tests on unchanged main and both branches; the CPU selection above excludes camera-related cases only, without modifying repository test settings. The existing axis-angle warning is unchanged. The complete repository suite, rendering, training runs, package builds and other operating systems were not tested. This corrects metadata for the existing reducers rather than changing reduction algorithms or validating arbitrary user-defined reducers.Upstream checks
At submission the CLA passed, the GitHub Actions Scan was queued, and the organization security workflow reported
action_required: https://github.com/google-deepmind/dm_control/actions/runs/36484669610. The test counts above are local results, not completed upstream CI.