-
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
feat(dataset): allow overriding tolerance_s #403
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -300,10 +300,10 @@ def record( | |
tags=None, | ||
num_image_writers=8, | ||
force_override=False, | ||
dataset_sync_tolerance_s=None, | ||
): | ||
# TODO(rcadene): Add option to record logs | ||
# TODO(rcadene): Clean this function via decomposition in higher level functions | ||
|
||
_, dataset_name = repo_id.split("/") | ||
if dataset_name.startswith("eval_") and policy is None: | ||
raise ValueError( | ||
|
@@ -634,6 +634,7 @@ def on_press(key): | |
episode_data_index=episode_data_index, | ||
info=info, | ||
videos_dir=videos_dir, | ||
tolerance_s=dataset_sync_tolerance_s, | ||
) | ||
if run_compute_stats: | ||
logging.info("Computing dataset statistics") | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's weird that this is the case as you are not using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no I don't unfortunately :( |
||
"--dataset-sync-tolerance-s", | ||
type=float, | ||
default=None, | ||
help="Override the maximum syncronisation tolerance (in seconds) between frames allowed by the LeRobot Dataset. Not passing an argument means we use FPS settings to infer the tolerance.", | ||
) | ||
|
||
parser_replay = subparsers.add_parser("replay", parents=[base_parser]) | ||
parser_replay.add_argument( | ||
"--fps", type=none_or_int, default=None, help="Frames per second (set to None to disable)" | ||
|
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 think the "right" way to handle this would be to have a private attribute
self._tolerance
which you set to1 / fps - 1e-4
in the__init__
(btwfps
should also be private as inself._fps
and then might need its own getter method). Then here you just need to returnself._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 thefps
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep no problem, happy to wait.