Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug Report] Box casting during init can cause invalid sampled values #768

Closed
1 task done
Jammf opened this issue Nov 7, 2023 · 5 comments · Fixed by #774
Closed
1 task done

[Bug Report] Box casting during init can cause invalid sampled values #768

Jammf opened this issue Nov 7, 2023 · 5 comments · Fixed by #774
Labels
bug Something isn't working

Comments

@Jammf
Copy link
Contributor

Jammf commented Nov 7, 2023

Describe the bug

During init, Box bounds are captured before casting to the destination dtype, but aren't checked again after the cast.

When Box casts to a lower-precision float with a range smaller than the source dtype, for example Box(0, 80000, dtype=np.float16), this can cause high and low to contain np.inf values without the corresponding bounded_above or bounded_below being False. This leads to an error when calling sample().

When Box casts np.inf from a float to an int, for example Box(255, np.inf, dtype=np.uint8), samples can under/overflow and be outside the Box bounds -- #328 seems to be related to this one.

Code example

>>> from gymnasium.spaces import Box
>>> import numpy as np
>>> np.finfo(np.float16).max
65500.0
>>> b = Box(0, 80000, dtype=np.float16)
<snip>/python3.11/site-packages/numpy/core/numeric.py:330: RuntimeWarning: overflow encountered in cast
  multiarray.copyto(a, fill_value, casting='unsafe')
>>> b
Box(0.0, inf, (1,), float16)
>>> b.bounded_below
array([ True])
>>> b.bounded_above
array([ True])
>>> b.sample()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<snip>/python3.11/site-packages/gymnasium/spaces/box.py", line 228, in sample
    sample[bounded] = self.np_random.uniform(
                      ^^^^^^^^^^^^^^^^^^^^^^^
  File "_generator.pyx", line 1043, in numpy.random._generator.Generator.uniform
OverflowError: Range exceeds valid bounds
>>>
>>>
>>> ### New session
>>> 
>>> from gymnasium.spaces import Box
>>> import numpy as np
>>> b = Box(255, np.inf, dtype=np.uint8)
<snip>/python3.11/site-packages/numpy/core/numeric.py:330: RuntimeWarning: overflow encountered in cast
  multiarray.copyto(a, fill_value, casting='unsafe')
>>> b
Box(255, 255, (1,), uint8)
>>> b.bounded_below
array([ True])
>>> b.bounded_above
array([False])
>>> b.sample()
array([2], dtype=uint8)  # overflow!

System info

  • installed in a venv using pip
  • gymnasium.__version__ == '0.29.1'
  • macOS Ventura 13.5.2
  • Python 3.11.3 (main, May 24 2023, 14:40:18) [Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin

Additional context

No response

Checklist

  • I have checked that there is no similar issue in the repo
@Jammf Jammf added the bug Something isn't working label Nov 7, 2023
@Jammf Jammf changed the title [Bug Report] Box casting can cause invalid sampled values [Bug Report] Box casting during init can cause invalid sampled values Nov 8, 2023
@Kallinteris-Andreas
Copy link
Collaborator

@pseudo-rnd-thoughts
Copy link
Member

My intuition is to raise an error within __init__ if either the low or high are outside of the dtype's possible values.
@RedTachyon @Jammf Do you agree? If so, could we make a PR that adds this?

@Jammf
Copy link
Contributor Author

Jammf commented Nov 8, 2023

Sure, I can make a PR. I think raising an error makes sense, but it'd also mean that integer boxes could no longer be unbounded (since ints can't be inf). Is that what we'd want?

Also, is that case, it seems like it'd have a lot of overlap with a MultiDiscrete space, so I'd wonder what the benefit of having both integer Box and MultiDiscrete would be (and the same with binary Box and MultiBinary). But, Box does also have a nice way to specify identical limits for every axis, so it has that going for it.

@pseudo-rnd-thoughts
Copy link
Member

We could treat inf as a special case to ignore as the max value to test are within the dtype's bound I think is the simplest approach to perverse backward compatibility

@pseudo-rnd-thoughts
Copy link
Member

Fixed in #774

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants