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

Fix ROS1 bridge type conversion bug #300

Merged
merged 3 commits into from
Sep 13, 2022
Merged

Fix ROS1 bridge type conversion bug #300

merged 3 commits into from
Sep 13, 2022

Conversation

tsampazk
Copy link
Collaborator

Closes #299

Also added some ifs to stop re-downloading existing files in yolov3 object detection learner download method .

@tsampazk tsampazk added bug Something isn't working test sources Run style checks test tools Test the toolkit methods labels Sep 12, 2022
@tsampazk tsampazk self-assigned this Sep 12, 2022
@tsampazk tsampazk added test sources Run style checks test tools Test the toolkit methods and removed test sources Run style checks test tools Test the toolkit methods labels Sep 12, 2022
passalis
passalis previously approved these changes Sep 13, 2022
View reviewed changes
Copy link
Collaborator

@passalis passalis 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!

stefaniapedrazzi
stefaniapedrazzi previously approved these changes Sep 13, 2022
View reviewed changes
Copy link
Collaborator

@stefaniapedrazzi stefaniapedrazzi 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

src/opendr/perception/object_detection_2d/yolov3/yolov3_learner.py Outdated Show resolved Hide resolved
@stefaniapedrazzi
Copy link
Collaborator

Just a question: is Thomas testing the master or develop version of the toolkit?

@tsampazk
Copy link
Collaborator Author

Just a question: is Thomas testing the master or develop version of the toolkit?

He is testing the develop branch with the updated nodes ROS1 nodes. I've been thinking about creating a tracker issue with a list of all ROS1 that still need to be updated, as in three of the issues Thomas opened mention inconsistent parameters, #302, #303, #304.

Right now many ROS1 nodes are updated in PRs #281 and #269, but many are still in their old state. I made a similar list on the ROS2 PR so we know where we stand #256. What do you think @stefaniapedrazzi ?

@stefaniapedrazzi
Copy link
Collaborator

stefaniapedrazzi commented Sep 13, 2022 •
edited
Loading

Just a question: is Thomas testing the master or develop version of the toolkit?

He is testing the develop branch with the updated nodes ROS1 nodes. I've been thinking about creating a tracker issue with a list of all ROS1 that still need to be updated, as in three of the issues Thomas opened mention inconsistent parameters, #302, #303, #304.

Right now many ROS1 nodes are updated in PRs #281 and #269, but many are still in their old state. I made a similar list on the ROS2 PR so we know where we stand #256. What do you think @stefaniapedrazzi ?

Yes it seems a good idea to list all ROS2 nodes that still needs to be updated in a single issue.

@tsampazk
Copy link
Collaborator Author

Yes it seems a good idea to list all ROS2 nodes that still needs to be updated in a single issue.

Sorry, i meant ROS1 nodes as the progress is not documented anywhere right now, i suppose that's still ok? Or should i create an issue for ROS2 nodes as well?

@stefaniapedrazzi
Copy link
Collaborator

No, I made a typo when writing. I meant ROS1.

stefaniapedrazzi
stefaniapedrazzi approved these changes Sep 13, 2022
View reviewed changes
Copy link
Collaborator

@stefaniapedrazzi stefaniapedrazzi 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!

@tsampazk tsampazk merged commit 021b948 into develop Sep 13, 2022
@tsampazk tsampazk deleted the fix-ros1-yolov3 branch September 13, 2022 09:14
@tsampazk tsampazk mentioned this pull request Sep 13, 2022
Closed
lucamarchionni pushed a commit to lucamarchionni/opendr that referenced this pull request Jun 10, 2024
@tsampazk
Fix ROS1 bridge type conversion bug (opendr-eu#300)
7094b5a 
* Fixed type conversion bug

* Added conditionals in download method to not re-download existing files

* Simplified else-ifs as suggested by review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@passalis passalis passalis approved these changes

@stefaniapedrazzi stefaniapedrazzi stefaniapedrazzi approved these changes

@omichel omichel Awaiting requested review from omichel omichel is a code owner

@ad-daniel ad-daniel Awaiting requested review from ad-daniel

Assignees

@tsampazk tsampazk

Labels
bug Something isn't working test sources Run style checks test tools Test the toolkit methods
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tsampazk @stefaniapedrazzi @passalis