-
Notifications
You must be signed in to change notification settings - Fork 641
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
Fix real-world configs by adding latency
#242
base: main
Are you sure you want to change the base?
Conversation
latency
6bf8d7a
to
d15f8f4
Compare
34622cd
to
be51483
Compare
@@ -1,6 +1,12 @@ | |||
# @package _global_ | |||
|
|||
fps: 15 | |||
# The latency corresponds to the delay in seconds between the current time at which the observation |
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.
Okay so, based on what we've discussed offline. I'm not sure about this.
- My top-level feedback is we shouldn't include latency as a separate variable.
Why?
- Based on our discussion: we might not even want to account for the latency (eg: using chunking). By putting this variable here, you are making a strong suggestion to the user about how they should use delta_timestamps. To do chunking (in the way that I've suggested: starting the chunk from the current timestep or even earlier) they would have to "lie" about the latency, or change delta timestamps to not use it (in which case you have a free parameter doing nothing). This is not a far fetched scenario at all. I would like to try it soon!
- (minor) From a pure coding design perspective, it's used in 1 place yet you've assigned it far away from that place. In general, if you insist on adding another variable to the namespace, it should be defined right next to where it's used.
- Relatively minor compared to 1. The way you document this is making some assumptions that we don't need to impose on users:
- They necessarily do inference on the rising edge of a clock which is also the same clock used to execute the action.
- They use a sleep in their code! (it's not so harmful to write this, but it can be a bit confusing like "wait - who's putting a sleep where?")
Not to assume up-front that you agree, but for efficiency, here's what I'd suggest if you do agree. Drop latency
, put it directly in the delta_timestamps
, and explain what you did there (and drop it completely from sim configs).
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 discussed with @aliberts . We think the proposed design of this PR is good.
This latency
variable should exists because:
- it is explicit
- it is used downstream by the environment
- in real world Dora, it is used for the dynamic sleep
- in sim Aloha, Pusht, Xarm, at some point, it could be used to simulate the latency
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.
Thanks @Cadene.
If it's used elsewhere, I agree it should stay in some form.
Btw can you please share a snippet of how it is used in the Dora env? That might help a lot with this conversation. Like for example, am I allowed to change this param to 2/fps or some other number? Is this a parameter or a constant?
What this does
latency
in config, set to 0 for simulation environments and 1/fps for real-world environmentHow it was tested
How to checkout & try? (for the reviewer)
This change is