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.

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

Bootcamp - Catherine Jin #113

Closed
wants to merge 13 commits into from
Closed

Bootcamp - Catherine Jin #113

wants to merge 13 commits into from

Conversation

cathjin
Copy link

@cathjin cathjin commented Oct 2, 2024

No description provided.

Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Reviewed


# Loop over the boxes list and create a list of bounding boxes
bounding_boxes = []
for r in predictions:
Copy link
Member

Choose a reason for hiding this comment

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

You don't really need to do this, since we only predicted 1 thing (image), so it will return only 1 results object (for our 1 image). The reason it has a list is because sometimes you run predict on a list of images, then it will return a list of results objects. https://docs.ultralytics.com/modes/predict/#working-with-results


# Plot the annotated image from the Result object
# Include the confidence value
image_annotated = prediction.plot()
Copy link
Member

Choose a reason for hiding this comment

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

Please also include/print the confidence value onto the image

# and convert to a numpy array
boxes_cpu = boxes_xyxy.detach().cpu().numpy()
# Loop over the boxes list and create a list of bounding boxes
for i in enumerate(boxes_cpu):
Copy link
Member

Choose a reason for hiding this comment

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

If you are using enumerate, the typical usage is for i, box in enumerate(boxes_cpu). You can tuple unpack in the for statement itself.

for i in enumerate(boxes_cpu):
index = i[0]
(x_min, y_min, x_max, y_max) = boxes_cpu[index]
bounding_box_obj = bounding_box.BoundingBox.create(
Copy link
Member

Choose a reason for hiding this comment

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

You can directly pass in the boxes_cpu[index] object, no need to remake a numpy array

bounding_box_obj = bounding_box.BoundingBox.create(
np.array([x_min, y_min, x_max, y_max])
)[1]
bounding_boxes.append(bounding_box_obj)
Copy link
Member

Choose a reason for hiding this comment

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

Please check the BoundingBox class, it returns a tuple.

modules/bootcamp/decision_simple_waypoint.py Show resolved Hide resolved
"""
Finds the distance to a landing pad
"""
distance = ((landing_pad_x - position_x) ** 2 + (landing_pad_y - position_y) ** 2) ** (1 / 2)
Copy link
Member

Choose a reason for hiding this comment

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

Can you think of how not to use the square root function? It is more computationally heavy, and we do not need to use it

and report.position.location_y == self.waypoint.location_y
):
command = commands.Command.create_halt_command()
min_distance = 100000000000
Copy link
Member

Choose a reason for hiding this comment

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

Please use float("inf") instead of an arbitrary large number

destination.location_x - report.position.location_x,
destination.location_y - report.position.location_y,
)
elif report.status == report.status.HALTED and report.position == destination:
Copy link
Member

Choose a reason for hiding this comment

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

Again, this is good, but what happens if the drone stops on the way to the waypoint? Or maybe it stopped on the way to the landing pad?

Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Reviewed

and 0 <= abs(report.position.location_y) < abs(self.waypoint.location_y)
):
command = commands.Command.create_set_relative_destination_command(
self.waypoint.location_x, self.waypoint.location_y
Copy link
Member

Choose a reason for hiding this comment

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

Please note that this is a relative distance command, so your position matters. Before, you checked that you were at (0,0), but now you may not be at (0,0).

if (
report.status == report.status.HALTED
and 0 <= abs(report.position.location_x) < abs(self.waypoint.location_x)
and 0 <= abs(report.position.location_y) < abs(self.waypoint.location_y)
Copy link
Member

Choose a reason for hiding this comment

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

Please use euclidean distance and not manhattan distance. Also, I'm a little confused as to why we are comparing absolute value of drone position with the absolute value of waypoint location? what happens if drone is at (10, 10) and waypoint is (5 ,5)?

@@ -70,8 +110,59 @@ def run(

# Do something based on the report and the state of this class...

if (
report.status == report.status.HALTED
and 0 <= abs(report.position.location_x) < abs(self.waypoint.location_x)
Copy link
Member

Choose a reason for hiding this comment

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

Please use euclidean distance

and 0 <= abs(report.position.location_y) < abs(self.waypoint.location_y)
):
command = commands.Command.create_set_relative_destination_command(
self.waypoint.location_x, self.waypoint.location_y
Copy link
Member

Choose a reason for hiding this comment

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

Relative distance

Comment on lines 124 to 129
self.waypoint.location_x
<= report.position.location_x
< closest_landing_pad(report, landing_pad_locations).location_x
or self.waypoint.location_x
>= report.position.location_x
> closest_landing_pad(report, landing_pad_locations).location_x
Copy link
Member

Choose a reason for hiding this comment

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

Again, please calculate the distances between the points rather than comparing their coordinates. You can compare distance squared, if that's what you were worried about. (a < b) => (a^2 < b^2) for any positive real number

- closest_landing_pad(report, landing_pad_locations).location_x
)
)
< 0.1
Copy link
Member

Choose a reason for hiding this comment

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

There is a self.accpetance_radius, which specifies how close the drone should get to the landing pad / waypoint

# Loop over the boxes list and create a list of bounding boxes
bounding_boxes = []
for _, boxes in enumerate(boxes_cpu):
box = bounding_box.BoundingBox.create(boxes)[1]
Copy link
Member

Choose a reason for hiding this comment

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

Please use tuple unpacking here, we don't want to throw away the information (boolean) if the create was successful or not. We want to handle the case in which the create failed

modules/bootcamp/decision_waypoint_landing_pads.py Outdated Show resolved Hide resolved
Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Reviewed

Comment on lines 115 to 120
command = commands.Command.create_set_relative_destination_command(
closest_landing_pad(report, landing_pad_locations).location_x
- report.position.location_x,
closest_landing_pad(report, landing_pad_locations).location_y
- report.position.location_y,
)
Copy link
Member

Choose a reason for hiding this comment

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

You can just save closest landing pad to a temporary variable instead of calculating it twice and writing all of this


elif (
report.status == report.status.HALTED
and self.waypoint.location_x == report.position.location_x
Copy link
Member

Choose a reason for hiding this comment

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

Please use your distance and acceptance radius to determine if you have arrived at waypoint rather than this

)
elif report.status == report.status.HALTED and self.arrived_waypoint is False:
command = commands.Command.create_set_relative_destination_command(
self.waypoint.location_x, self.waypoint.location_y
Copy link
Member

Choose a reason for hiding this comment

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

Relative distance command

predictions = ...
bounding_boxes = []
predictions = self.__model.predict(
source=image, show=False, conf=0.7, device="cpu", verbose=True
Copy link
Member

Choose a reason for hiding this comment

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

Rather than hard-coding device, see line 39: __DEVICE

bounding_boxes = []
for i in range(0, boxes_xyxy.shape[0]):
result, box = bounding_box.BoundingBox.create(boxes_cpu[i])
if result is True:
Copy link
Member

Choose a reason for hiding this comment

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

You can also just leave it as if result, since result is a boolean

yolov8n.pt Outdated
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need to download another model, the bootcamp contains one

Copy link
Member

@maxlou05 maxlou05 left a comment

Choose a reason for hiding this comment

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

Approved

@maxlou05 maxlou05 closed this Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants