Skip to content

Keep the scaled action space valid for a negative scale - #274

Open
DenisDrobyshev wants to merge 1 commit into
Farama-Foundation:mainfrom
DenisDrobyshev:scale-actions-negative
Open

Keep the scaled action space valid for a negative scale#274
DenisDrobyshev wants to merge 1 commit into
Farama-Foundation:mainfrom
DenisDrobyshev:scale-actions-negative

Conversation

@DenisDrobyshev

Copy link
Copy Markdown

Description

scale_actions_v0 builds the scaled action space by multiplying both bounds in
place:

return Box(low=act_space.low * scale, high=act_space.high * scale)

A negative scale maps [low, high] onto [high * scale, low * scale], so the
two bounds swap places and the low ends up above the high. Gymnasium then
refuses the space from inside its own constructor:

env = supersuit.scale_actions_v0(gym.make("Pendulum-v1"), -1.0)
ValueError: Box all low values must be less than or equal to high (some values break this)

The message names neither scale_actions_v0 nor its scale argument, so the
report points at Gymnasium rather than at the wrapper that built the space.

The bounds are now ordered with an element-wise minimum and maximum, which is
the image of the interval under the scaling. Nothing changes for a positive
scale, where low * scale is already the smaller of the two.

scale before after
2.0 Box(-4.0, 4.0) Box(-4.0, 4.0)
0.5 Box(-1.0, 1.0) Box(-1.0, 1.0)
0.0 Box(0.0, 0.0) Box(0.0, 0.0)
-1.0 ValueError Box(-2.0, 2.0)
-2.5 ValueError Box(-5.0, 5.0)

The dtype is left to Box to infer exactly as before, so integer action spaces
are unaffected by this change.

Tests

test_scale_action_wrapper_negative_scale in test/aec_mock_test.py, next to
the existing test_scale_action_wrapper. It uses an asymmetric action space,
Box(1.0, 5.0), so the swap is visible: with scale=-2.0 the wrapped space is
Box(-10.0, -2.0), and the action [2, 1, 3] arrives at the environment as
[-4, -2, -6]. It fails on main with the ValueError above and passes here.

The existing test only covers positive factors — 2, 0.5 and 0.3 — which is why
this went unnoticed.

Full suite locally: 106 passed against 105 on main, the difference being this
test; the one failure and six collection errors are identical on both sides and
come from optional dependencies missing in my environment. pre-commit run is
clean on both files.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have run the pre-commit checks with pre-commit run --all-files
  • I have added tests that prove my fix is effective

scale_actions_v0 built its Box as Box(low=low * scale, high=high * scale).
A negative scale maps [low, high] onto [high * scale, low * scale], so the two
bounds swap places and the low ends up above the high. Gymnasium then rejects
the space from inside its own constructor:

    ValueError: Box all low values must be less than or equal to high

which names neither this wrapper nor its scale argument. The bounds are now
ordered with an element-wise minimum and maximum, which is the image of the
interval under the scaling.

The existing test only covers positive factors, 2, 0.5 and 0.3.
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