-
Notifications
You must be signed in to change notification settings - Fork 14
Fixed merge errors #78
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
Conversation
Signed-off-by: Dongyun Kim <[email protected]>
Signed-off-by: Dongyun Kim <[email protected]>
Summary of ChangesHello @DongyunRobotis, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the data merging process, specifically concerning the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request addresses an important bug in dataset merging where task_index values could collide. The introduction of _build_merged_task_mapping is a good approach to create a unified mapping before processing the files. My review includes a suggestion for a more efficient implementation using pandas, a type hint improvement for better code clarity, and a high-severity comment about an incomplete fix that could lead to inconsistent metadata. Overall, the core logic of the fix is sound, but requires a bit more work to be complete and robust.
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
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.
Pull request overview
This PR fixes a bug in the dataset merging logic where task_index values were being incorrectly merged based on the first episode's task index. The fix introduces a new task mapping system that correctly remaps task indices across merged datasets by task name, ensuring consistency when multiple datasets with potentially overlapping task indices are combined.
Key changes:
- Added
_build_merged_task_mapping()method to create a mapping between old and new task indices based on task names - Updated
_copy_parquet_and_update_indices_for_merge()to apply the task index mapping when processing parquet files - Bumped version to 0.7.1 across all packages
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| physical_ai_server/physical_ai_server/data_processing/data_editor.py | Implements the core fix by building task index mappings and applying them during parquet merge operations |
| rosbag_recorder/package.xml | Version bump to 0.7.1 |
| rosbag_recorder/CHANGELOG.rst | Added changelog entry for 0.7.1 |
| physical_ai_tools/package.xml | Version bump to 0.7.1 |
| physical_ai_tools/CHANGELOG.rst | Added changelog entry describing the task_index merge fix |
| physical_ai_server/setup.py | Version bump to 0.7.1 |
| physical_ai_server/package.xml | Version bump to 0.7.1 |
| physical_ai_server/CHANGELOG.rst | Added changelog entry describing the task_index merge fix |
| physical_ai_manager/package.json | Version bump to 0.7.1 |
| physical_ai_manager/CHANGELOG.rst | Added changelog entry for 0.7.1 |
| physical_ai_interfaces/package.xml | Version bump to 0.7.1 |
| physical_ai_interfaces/CHANGELOG.rst | Added changelog entry for 0.7.1 |
| Isaac-GR00T | Added subproject commit reference |
Files not reviewed (1)
- physical_ai_manager/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Dongyun Kim <[email protected]>
Signed-off-by: Dongyun Kim <[email protected]>
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.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- physical_ai_manager/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Dongyun Kim <[email protected]>
Signed-off-by: Dongyun Kim <[email protected]>
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.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- physical_ai_manager/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
physical_ai_server/physical_ai_server/data_processing/data_editor.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Dongyun Kim <[email protected]>
ola31
left a comment
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.
Good
Seongoo
left a comment
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.
LGTM
ola31
left a comment
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.
@DongyunRobotis Please update versions to 0.7.2 🙏
Signed-off-by: Dongyun Kim <[email protected]>
Signed-off-by: Dongyun Kim <[email protected]>
Fixed an issue where the task_index was being merged based on the first episode when merging episodes in the .parquet data.