-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[RLlib] Issue: Get single step input dict incorrect. #20217
[RLlib] Issue: Get single step input dict incorrect. #20217
Conversation
…e_get_single_step_input_dict_incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just 3 simple comments. 1 comment is repeated 5 times :)
thanks.
}) | ||
input_dict = batch.get_single_step_input_dict( | ||
view_requirements=view_reqs, index="last") | ||
check( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertEqual?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our check()
works on nested structures and also handles np.array vs list comparisons, so it's much more practical.
}) | ||
input_dict = batch.get_single_step_input_dict( | ||
view_requirements=view_reqs, index="last") | ||
check( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
}) | ||
input_dict = batch.get_single_step_input_dict( | ||
view_requirements=view_reqs, index="last") | ||
check( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
}) | ||
input_dict = batch.get_single_step_input_dict( | ||
view_requirements=view_reqs, index="last") | ||
check( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
}) | ||
input_dict = batch.get_single_step_input_dict( | ||
view_requirements=view_reqs, index="last") | ||
check( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
def get_single_step_input_dict( | ||
self, | ||
view_requirements: ViewRequirementsDict, | ||
index: Union[str, int] = "last", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like index can be a tuple too. maybe update the annotation and doc string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took out the Tuple option. Never used and doesn't seem to make any sense here.
We only ever use "last" or -1 (in the MARWIL algo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
data = self[view_col][-1] | ||
traj_len = len(self[data_col]) | ||
missing_at_end = traj_len % view_req.batch_repeat_value | ||
obs_shift = -1 if data_col in [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you help comment what obs_shift means?? I can't quite understand it. thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool. didn't notice is our own util.
Fixes issue #20216
A new unit test has been added to the CI to cover this functionality in the future.
Why are these changes needed?
Related issue number
Checks
Closes #20216
scripts/format.sh
to lint the changes in this PR.