Skip to content

Accept numpy integers for rollout nstep and chunk_size#3300

Open
Nas01010101 wants to merge 1 commit into
google-deepmind:mainfrom
Nas01010101:fix/rollout-accept-numpy-integers
Open

Accept numpy integers for rollout nstep and chunk_size#3300
Nas01010101 wants to merge 1 commit into
google-deepmind:mainfrom
Nas01010101:fix/rollout-accept-numpy-integers

Conversation

@Nas01010101
Copy link
Copy Markdown

Problem

rollout rejects nstep and chunk_size arguments whose type is a numpy integer (e.g. np.int64):

if nstep and not isinstance(nstep, int):
    raise ValueError('nstep must be an integer')
if chunk_size and not isinstance(chunk_size, int):
    raise ValueError('chunk_size must be an integer')

isinstance(np.int64(3), int) is False, so a numpy integer is rejected even though it is a valid integer. These values very commonly come from numpy operations (array.shape[i], products of dimensions, np.arange(...)[i], etc.), so this raises ValueError: nstep must be an integer:

rollout.rollout(model, data, initial_state, nstep=np.int64(3))

while the identical call with a Python int works.

Fix

Accept numpy.integer in addition to the built-in int for both checks (numpy is already imported as np). The downstream C++ call already handles the value numerically, so no other change is needed.

Tests

Adds test_numpy_integer_nstep_and_chunk_size, which calls rollout with np.int64 values for nstep and chunk_size. It fails before this change (ValueError) and passes after; the rest of rollout_test.py remains green.

`rollout` rejected `nstep` and `chunk_size` arguments whose type was a numpy
integer (e.g. `np.int64`), because `isinstance(np.int64(3), int)` is `False`.
These values commonly come from numpy operations such as `array.shape[i]`, so
passing them raised `ValueError: nstep must be an integer` even though they are
valid integers.

Accept `numpy.integer` in addition to the built-in `int` for both checks. Adds a
regression test passing `np.int64` for `nstep` and `chunk_size`; it fails before
this change and passes after.
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