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.

Sign up for GitHub

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

Jump to bottom

Implement ROS interface for live spot-aria with shared coordinate system and visualization in rviz #110

Merged
merged 1 commit into from
Jan 20, 2024
Merged

Implement ROS interface for live spot-aria with shared coordinate system and visualization in rviz #110

merged 1 commit into from
Jan 20, 2024

Conversation

KavitShah1998
Copy link
Contributor

No description provided.

@KavitShah1998 KavitShah1998 self-assigned this Dec 12, 2023
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 12, 2023
zephirefaith
zephirefaith requested changes Dec 12, 2023
View reviewed changes
aria_data_loaders/aria_data_utils/aria_live_streamer.py Outdated Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Show resolved Hide resolved
bd_spot_wrapper/spot_wrapper/spot.py Outdated Show resolved Hide resolved
spot_rl_experiments/spot_rl/envs/skill_manager.py Outdated Show resolved Hide resolved
spot_rl_experiments/spot_rl/launch/local_only.sh Show resolved Hide resolved
spot_rl_experiments/spot_rl/utils/helper_nodes.py Outdated Show resolved Hide resolved
spot_rl_experiments/spot_rl/utils/robot_subscriber.py Outdated Show resolved Hide resolved
KavitShah1998
KavitShah1998 commented Dec 13, 2023
View reviewed changes
.gitmodules Outdated Show resolved Hide resolved
jimmytyyang
jimmytyyang reviewed Dec 20, 2023
View reviewed changes
Copy link
Contributor

@jimmytyyang jimmytyyang left a comment

Choose a reason for hiding this comment

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

Thank you so much for this PR. Overall, I think they look good! But one concern I have is that will this affect the existing skill execution? For example, for users who only want to do development without using Aria, will this PR introduce dependency to Aria installation? I can see some files import Aria stuff. Is it possible to install spot-sm2real only the skill part without installing Aria code for the ease of downstream usecases? I just want to ensure that this PR is well-tested in terms of installation and package dependency. Thank you!

aria_data_loaders/aria_data_utils/aria_live_streamer.py Outdated Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Outdated Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_streamer.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/episodic_memory_robotic_fetch.py Show resolved Hide resolved
spot_rl_experiments/spot_rl/launch/local_only.sh Show resolved Hide resolved
spot_rl_experiments/spot_rl/models/owlvit.py Show resolved Hide resolved
rutadesai
rutadesai approved these changes Jan 12, 2024
View reviewed changes
Copy link

@rutadesai rutadesai left a comment

Choose a reason for hiding this comment

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

Added some comments -- mostly small nits. Looks good overall. Really appreciated all the docstrings and detailed comments and things organized nicely in their own classes and utils files. Great job! Let's get this landed soon :)

README.md Outdated Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Outdated Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Outdated Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_streamer.py Show resolved Hide resolved
bd_spot_wrapper/spot_wrapper/spot.py Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
spot_rl_experiments/configs/ros_frame_names.yaml Show resolved Hide resolved
spot_rl_experiments/spot_rl/models/owlvit.py Show resolved Hide resolved
spot_rl_experiments/spot_rl/envs/skill_manager.py Show resolved Hide resolved
akshararai
akshararai reviewed Jan 12, 2024
View reviewed changes
Copy link
Contributor

@akshararai akshararai left a comment

Choose a reason for hiding this comment

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

done until aria_data_loaders/aria_data_utils/episodic_memory_robotic_fetch.py
Need to stop, this PR is really tooooo long to be reviewed properly.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_streamer.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/conversions.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/detector_wrappers/object_detector.py Outdated Show resolved Hide resolved
aria_data_loaders/aria_data_utils/episodic_memory_robotic_fetch.py Show resolved Hide resolved
@zephirefaith zephirefaith mentioned this pull request Jan 17, 2024
Closed
@zephirefaith zephirefaith self-assigned this Jan 17, 2024
@zephirefaith zephirefaith mentioned this pull request Jan 17, 2024
Open
rutadesai
rutadesai approved these changes Jan 19, 2024
View reviewed changes
Copy link

@rutadesai rutadesai left a comment

Choose a reason for hiding this comment

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

WOOHOO! Looking good :) Let's get this landed!

aria_data_loaders/aria_data_utils/episodic_memory_robotic_fetch.py Show resolved Hide resolved
spot_rl_experiments/spot_rl/envs/skill_manager.py Show resolved Hide resolved
aria_data_loaders/aria_data_utils/aria_live_streamer.py Show resolved Hide resolved
@KavitShah1998 KavitShah1998 mentioned this pull request Jan 20, 2024
Open
6 tasks
@KavitShah1998
Copy link
Contributor Author

All major comments on the PR have been addressed.

Un-addressed comments have been resolved and marked in the issue - #129.

Merging this PR (#110) into main.

Thank you all!

@KavitShah1998 KavitShah1998 merged commit 5315f13 into main Jan 20, 2024
2 checks passed
@KavitShah1998 KavitShah1998 deleted the aria_streamer_live branch January 20, 2024 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshararai akshararai akshararai left review comments

@rutadesai rutadesai rutadesai approved these changes

@zephirefaith zephirefaith zephirefaith approved these changes

@tusharsangam tusharsangam tusharsangam approved these changes

@jimmytyyang jimmytyyang Awaiting requested review from jimmytyyang

Assignees

@zephirefaith zephirefaith

@KavitShah1998 KavitShah1998

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@KavitShah1998 @akshararai @rutadesai @zephirefaith @tusharsangam @jimmytyyang @facebook-github-bot