[rlib] Independent bound for each dimension AssertionError #16845 #16860
Conversation
Restored support for Box space type with independent bound for each dimension.
rllib/models/catalog.py
Outdated
| @@ -241,8 +241,8 @@ def get_action_dist( | |||
| if action_space.dtype.name.startswith("int"): | |||
| low_ = np.min(action_space.low) | |||
| high_ = np.max(action_space.high) | |||
| assert np.all(action_space.low == low_) | |||
| assert np.all(action_space.high == high_) | |||
| assert np.all(action_space.low >= low_) | |||
There was a problem hiding this comment.
Hi @frenkowski and @richardliaw,
I am looking at these assertions and wondering if they are even needed now. How they would be false? I cannot think of how but I am probably missing something. Do you have an example?
There was a problem hiding this comment.
Sure!
If you have a Box with independent bound for each dimension (e.g. Box(low=np.array([-1.0, -2.0]), high=np.array([2.0, 4.0]), dtype=np.float32)), the lowest value is -2.0.
Therefore in this case the assertion fails, given that -1.0 != -2.0 (but -1.0 >= -2.0).
The same obviously applies to the highest value.
There was a problem hiding this comment.
Hi @frenkowski,
Thank you for the reply. I think I may not have been clear. I agree that the original issue you posted is a bug what I am wondering is after your fix would it ever assert.
You defined low_ to be np.min(action_space.low) then you have an assertion to check that all values in action_space.low are greater than our equal to that value. By construction that is the lowest value so how could that ever be false?
I am not saying the logic of your solution is wrong I am just wondering what kind of issue that new assertion could catch.
Edit: I guess what I am wondering is if we can just remove the assertions.
There was a problem hiding this comment.
@mvindiola1 I apologize for the misunderstanding!
I totally agree with you, we can just remove the assertions regardless of the bug.
I suggested that change because I sincerely do not know the logic of the author with respect to the original assertion.
There was a problem hiding this comment.
I guess it is up to @richardliaw as to which approach he prefers.
There was a problem hiding this comment.
hmm, agree with @mvindiola1 that we can just remove the assertions!
There was a problem hiding this comment.
@mvindiola1 @richardliaw I removed the assertions with the new commit
|
@frenkowski ah! sorry about the late approval, and thanks a bunch for the ping! |
Why are these changes needed?
Restored support for Box space type with independent bound for each dimension.
Related issue number
#16845
cc @richardliaw
Checks
scripts/format.shto lint the changes in this PR.