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

Evan Zhao's bootcamp #95

Closed
wants to merge 9 commits into from
Closed

Evan Zhao's bootcamp #95

wants to merge 9 commits into from

Conversation

Evang264
Copy link

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

# Do something based on the report and the state of this class...
dx = self.waypoint.location_x - report.position.location_x
dy = self.waypoint.location_y - report.position.location_y
if dx**2 + dy**2 > self.acceptance_radius**2:
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 dx and dy, a more descriptive variable such as distance_to_waypoint_x would be better. Don't be afraid of long variable names!


def _calc_relative_dist(
self, loc1: location.Location, loc2: location.Location
) -> "tuple[int, int]":
Copy link
Member

Choose a reason for hiding this comment

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

Great use of doc strings and type hinting! Although in this case we would be using floats and not only integers

)
else:
# otherwise, we go back to the origin
dx, dy = -report.position.location_x, -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.

Great job handling the edge cases!
(You do not need to change anything here)

best_index, shortest_dist = 0, self._calc_distance_squared(
report, landing_pad_locations[0]
)
for i in range(1, len(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.

Great job recognizing that you can skip iterating through the first one!
(You do not need to change anything here)

Comment on lines +78 to +81
if self.achieved_waypoint:
# now we find the index of the landing pad closest to the drone
# while keeping that landing pad's distance (squared)

Copy link
Member

Choose a reason for hiding this comment

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

Although this works, it recalculates the closest landing pad constantly when travelling towards the closest landing pad. This is sort of wasted computation, since the closest landing pad will not change if you are travelling in a straight line towards it. Try to find a way to avoid this


# Detach the xyxy boxes to make a copy,
# move the copy into CPU space,
# and convert to a numpy array
boxes_cpu = ...
boxes_cpu = boxes_xyxy.cpu().numpy()
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 make a copy by using detach() before cpu()

for i in range(0, boxes_cpu.shape[0]):
successful, box = bounding_box.BoundingBox.create(bounds=boxes_cpu[i])
if successful:
bounding_boxes.append(box)
Copy link
Member

Choose a reason for hiding this comment

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

In statistics, we usually discard the entire data point (the whole image) if part of it is broken (this bounding box), instead of only discarding the broken part.
(Just something to think about, you do not need to change anything here)

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha, thanks for letting me know!

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

# now we find the index of the landing pad closest to the drone
# while keeping that landing pad's distance (squared)

nearest_landing_pad = self._find_best_landing_pad(report, 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.

I mean you can move this find_best_landing_pad function to a different part, like the one right before this section.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, can you elaborate on what you mean here? Do you mean refactor the code by moving the function _find_best_landing_pad() to right before run()?

@@ -68,10 +69,81 @@ def run(
# ↓ BOOTCAMPERS MODIFY BELOW THIS COMMENT ↓
# ============

# Do something based on the report and the state of this class...
if not self.achieved_waypoint:
dx, dy = self._calc_relative_dist(report.position, self.waypoint)
Copy link
Member

Choose a reason for hiding this comment

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

You could use a more descriptive variable name here, like in the first task.

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 Sep 22, 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