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

Continuous Door Opening Actions and Fixed Light in GridRowDoor #1680

Merged
merged 11 commits into from
Jul 3, 2024

Conversation

matrixbalto
Copy link
Collaborator

Added continuous door opening actions, MoveKey and TurnKey, and set the TurnOnLight action to always turn on the light successfully.

Copy link
Member

@NishanthJKumar NishanthJKumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Left a few comments and questions just to make sure I understand everything/it's as simple as it can be (to help with debugging!)

Comment on lines 343 to 351
if robot_cell == door_cell and not (door_target - 0.1 \
<= door_open <= door_target + 0.1 \
and door_target1 - 0.1 <= door_open1 <= door_target1 + 0.1):
new_door_level = np.clip(
state.get(self._door, "open") + ddoor, 0.0, 1.0)
next_state.set(self._door, "open", new_door_level)
new_door1_level = np.clip(
state.get(self._door, "open1") + ddoor1, 0.0, 1.0)
next_state.set(self._door, "open1", new_door1_level)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, so is it true that the robot could technically open the door with just one action (by setting the door level and door1 level correctly simultaneously?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there isn't an action that handles both of the levels at once so it's forced to open the door through two actions (one TurnKey action and one MoveKey action)

Comment on lines +359 to +361
if robot_cell == light_cell and dlight == 0.75 and self._LightOff_holds(
state, [self._light]):
next_state.set(self._light, "level", 0.75)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait - so is it now true that the dlight part of the action vector is effectively useless? If that's the case, I'd prefer you just modify the sampler to be correct instead of handling the light in this way in the transition function: modifying the sampler to always succeed is more straightforward and clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see you've also modified the sampler, so why do you also need this line in the transition function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait sorry I don't quite understand, don't we still need to change the light level and to do so we first need to check that the robot is currently in the light cell, if a TurnOnLight was called (dlight == 0.75)? I guess I could technically get rid of the self._LightOff_holds(state, [self._light]), but I think we'll need to check that the light is off when we generalize to multiple lights

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wait nvm, I missed the dlight == 0.75 part; my bad!

predicators/envs/grid_row.py Outdated Show resolved Hide resolved
Copy link
Member

@NishanthJKumar NishanthJKumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Left one final comment - please address before merging!

assert option.terminal(state)
if all(a.holds(state) for a in ground_nsrt.add_effects):
break
print(state)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove print!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops sorry about that!!

@matrixbalto matrixbalto merged commit 8272c02 into master Jul 3, 2024
6 checks passed
@matrixbalto matrixbalto deleted the fixed_light branch July 3, 2024 17:51
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.

2 participants