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

Teighan Miller - Bootcamp #142

Closed
wants to merge 19 commits into from
Closed

Conversation

teighanmiller
Copy link

No description provided.

Copy link
Contributor

@TongguangZhang TongguangZhang left a comment

Choose a reason for hiding this comment

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

reviewed

@@ -68,6 +66,25 @@ def run(
# ↓ BOOTCAMPERS MODIFY BELOW THIS COMMENT ↓
# ============

# If the drone is halted and not at the destination, move the drone to destination
if report.status == drone_status.DroneStatus.HALTED and report.destination != self.waypoint:
Copy link
Contributor

Choose a reason for hiding this comment

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

use the acceptance radius, not != (float operations are a little funky, so you can't do a direct comparison like this)

Comment on lines 71 to 73
command = commands.Command.create_set_relative_destination_command(
self.waypoint.location_x, self.waypoint.location_y
)
Copy link
Contributor

Choose a reason for hiding this comment

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

note that it's relative distance, not absolute, so you can't just use the coords of the waypoint

Comment on lines 75 to 77
elif (
report.status == drone_status.DroneStatus.HALTED and report.destination == self.waypoint
):
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above - use acceptance radius instead of ==

Comment on lines 75 to 77
elif (
report.status == drone_status.DroneStatus.HALTED and report.destination == self.waypoint
):
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like you make a redundant calculation here - you can nest the if statements, which lets u do if else

if halted:
    if at location:
        ...
    else:
        ...

Comment on lines 81 to 87
if (
report.status == drone_status.DroneStatus.LANDED
and (report.position - report.destination) > self.acceptance_radius
):
command = commands.Command.create_set_relative_destination_command(
self.waypoint.location_x, self.waypoint.location_y
)
Copy link
Contributor

Choose a reason for hiding this comment

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

it doesn't make sense to land the drone and then take off again if it's in the wrong spot - just make sure it's in the right spot before landing


# Detach the xyxy boxes to make a copy,
# move the copy into CPU space,
# and convert to a numpy array
boxes_cpu = ...
boxes_cpu = prediction.boxes.xyxy.cpu().numpy()
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to call detach(), this removes it from the computational graph, which makes it stop tracking the gradients
basically, during training, gradients are tracked on the tensors for back propagation, but since we're only using inference, we don't need the gradients, and they take up a lot of memory, so we should stop tracking them


# Loop over the boxes list and create a list of bounding boxes
bounding_boxes = []
for i in range(0, boxes_cpu.shape[0]):
new_box = bounding_box.BoundingBox.create(boxes_xyxy[i])
Copy link
Contributor

Choose a reason for hiding this comment

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

better practice to do result, new_box = ...

Comment on lines 122 to 123
if new_box[0]:
bounding_boxes += new_box
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of skipping failed boxes, we acc wanna return an empty list - in data science, when part of a data point is corrupted, we skip the whole data point instead of returning partial data, in this case, 1 image is 1 data point, and the boxes are part of the image, so we skip the whole image if any of the boxes fail

for i in range(0, boxes_cpu.shape[0]):
new_box = bounding_box.BoundingBox.create(boxes_xyxy[i])
if new_box[0]:
bounding_boxes += new_box
Copy link
Contributor

Choose a reason for hiding this comment

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

use .append for lists - also, do you need to add the success flag to the bounding box list instead of just the box

Comment on lines 129 to 130
if not bounding_boxes:
return [], image_annotated
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be deleted if you don't add the success flags

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

@@ -68,6 +66,16 @@ def run(
# ↓ BOOTCAMPERS MODIFY BELOW THIS COMMENT ↓
# ============

# If the drone is halted and not at the destination, move the drone to destination
if report.status == drone_status.DroneStatus.HALTED:
if (report.destination - self.waypoint) < self.acceptance_radius:
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 this. I'm not even sure how this comparison works, as you are comparing a vector (in this case a class with 2 ints) with a scalar (1 int).

@@ -38,6 +38,8 @@ def __init__(self, waypoint: location.Location, acceptance_radius: float) -> Non
# ============

# Add your own
self.target = None
self.tol = 0.0001
Copy link
Member

Choose a reason for hiding this comment

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

The tolerance is given to you as acceptance_radius

if report.status == drone_status.DroneStatus.HALTED:
if (report.destination - self.waypoint) < self.acceptance_radius:
command = commands.Command.create_set_relative_destination_command(
abs(self.waypoint.location_x), 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.

It is a relative distance command, so your own position matters. Also, don't use abs because (-1, -1) is a valid location and is different from (1, 1)

if report.status == drone_status.DroneStatus.HALTED and report.destination != self.target:

command = commands.Command.create_set_relative_destination_command(
self.target.location_x, self.target.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, your position matters

Comment on lines 87 to 94
if (
report.status == drone_status.DroneStatus.LANDED
and (report.position - report.destination) > self.acceptance_radius
):
command = commands.Command.create_set_relative_destination_command(
self.target.location_x, self.target.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.

Instead of doing this extra case, can you just add to the condition of the elif above? This doesn't really work right now because the drone doesn't accept move commands after it lands, so incorporate it into the one above

closest destination. This location.Location object is returned.

Args:
current_location (location.Location): Location the drone is currently at (0, 0)
Copy link
Member

Choose a reason for hiding this comment

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

It starts at (0,0), but you cannot assume the drone doesn't move (you don't know where the drone is when you call this function). However, it doesn't really matter so it's fine. Returning to (0,0) is ok if there are no landing pads, great job for thinking about this case! (You do not need to change anything here)


# Loop over the boxes list and create a list of bounding boxes
bounding_boxes = []
for i in range(0, boxes_cpu.shape[0]):
result, new_box = bounding_box.BoundingBox.create(boxes_xyxy[i])
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 boxes_cpu here

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. Also check discord about the distance thing

Comment on lines 72 to 73
if self.target is None and report.status is drone_status.DroneStatus.HALTED:
self.target = self.find_closest_landing_pad(report.position, landing_pad_locations)
Copy link
Member

Choose a reason for hiding this comment

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

For task 4, you need to fly to the waypoint first (you can think of it as target1), before you can fly to the nearest landing form the waypoint (target2). So in total you need to go to 2 things, but you only go to 1 (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.

Conditionally approved! I would advise to be careful when using is, because I think that is just compares pointers and not compares objects (does not invoke the object's eq()). I think it works in your case since most object assignments are just references. This is just an FYI for future.

@maxlou05 maxlou05 closed this Jan 24, 2025
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.

3 participants