feat(dataset): allow overriding tolerance_s #403
Conversation
| @@ -126,6 +128,9 @@ def tolerance_s(self) -> float: | |||
| are not close enough from the requested frames. It is only used when `delta_timestamps` | |||
| is provided or when loading video frames from mp4 files. | |||
| """ | |||
| if self.tolerance_s is not None: | |||
There was a problem hiding this comment.
I think the "right" way to handle this would be to have a private attribute self._tolerance which you set to 1 / fps - 1e-4 in the __init__ (btw fps should also be private as in self._fps and then might need its own getter method). Then here you just need to return self._tolerance.
OR, if we don't want to bother with encapsulation, this getter should disappear and we should just have an attribute self.tolerance_s. The only danger here is that it's independent of the fps from an interface perspective.
@Cadene I think you should weigh in here as I know you're not a fan of using private attributes.
There was a problem hiding this comment.
@villekuosmanen After discussing internally, we will put your PR on hold for a bit while we think about a design.
cc @alexander-soare
There was a problem hiding this comment.
@villekuosmanen yeah so since I am encountering this right now with some RL stuff I am doing, we just want to get wait that working then use whatever it was I did to make it happen. It might be this approach, but there may be other ways of handling it.
Thanks a lot mate!
There was a problem hiding this comment.
yep no problem, happy to wait.
| @@ -798,7 +799,13 @@ def replay(robot: Robot, episode: int, fps: int | None = None, root="data", repo | |||
| nargs="*", | |||
| help="Any key=value arguments to override config values (use dots for.nested=overrides)", | |||
| ) | |||
|
|
|||
| parser_record.add_argument( | |||
There was a problem hiding this comment.
Does this matter? It gets lost in the hub upload anyway right? Then when someone wants to actually use the dataset they can set the tolerance as they wish.
There was a problem hiding this comment.
I was having an issue when calculating stats from the dataset at the end of record which is why I put it here. Happy to delete the arg if there is some other, better way to do it.
There was a problem hiding this comment.
It's weird that this is the case as you are not using delta_timestamps which is the only way I could see the tolerance guard is being triggered. Do you have an exception trace (only dig it up if you cbb, as this PR is on hold anyway).
There was a problem hiding this comment.
no I don't unfortunately :(
There was a problem hiding this comment.
Thanks @villekuosmanen!
I agree that setting tolerance may be helpful. In fact, I came across the same need myself just a couple of days ago.
I'm not entirely sure why it's being added in the record script, so I've got an inline comment on that.
What this does
My cameras can do maximum 30 FPS while I would like to record the joint states at higher FPS. In addition, occasional interrupts might cause an individual frame to be dropped or delayed, and having to drop the whole episode because of these may not be worth it.
This simply adds an option to override the tolerance_s constant in LeRobotDataset, and passes the value from data recording command line args.
How it was tested
Tested locally when recording data
How to checkout & try? (for the reviewer)
Record some episodes with a real robot. Test with the command line arg turned on and off.