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 Submission - Emily Qi #135

Closed
wants to merge 3 commits into from
Closed

Bootcamp Submission - Emily Qi #135

wants to merge 3 commits into from

Conversation

emlyqi
Copy link

@emlyqi emlyqi commented Jan 1, 2025

No description provided.

@emlyqi emlyqi changed the title tasks 1, 2, 3, 4 Bootcamp Submission - Emily Qi Jan 4, 2025
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

Comment on lines +126 to +127
if result:
bounding_boxes.append(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 want to return no boxes if any of then fail. This is because in data science, when we have corrupted data, we skip the whole data point instead of partial data - in this case, the image is the data point and the boxes are parts of the data

Comment on lines 50 to 53
def distance_between_locations(self, other_location: location.Location) -> float:
"""
Calculate the squared distance between two Location objects
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is the distance squared, name it accordingly

distance_from_waypoint <= self.acceptance_radius**2
and not self.has_sent_landing_command
):
smallest_distance = 999999999
Copy link
Contributor

Choose a reason for hiding this comment

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

it's generally bad practice to use just a very large number like this - try looking for how to represent infinity in python

Comment on lines 107 to 108
smallest_distance_x = 0
smallest_distance_y = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

consider storing the waypoint object instead of the x and y, this also makes is so you don't need to check ready_to_land to avoid landing the drone and (0, 0)

and not self.has_sent_landing_command
):

if distance_from_waypoint <= self.acceptance_radius**2:
Copy link
Contributor

Choose a reason for hiding this comment

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

we generally say that an acceptance radius is strictly <, not <=

command = commands.Command.create_land_command()
self.has_sent_landing_command = True

elif distance_from_waypoint > self.acceptance_radius**2:
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need the elif or can it just be an else?

command = commands.Command.create_land_command()
self.has_sent_landing_command = True

elif (
Copy link
Contributor

Choose a reason for hiding this comment

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

we generally say that an acceptance radius is strictly <, not <=

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

Comment on lines 91 to 93
distance_from_waypoint = (
self.waypoint.location_x - report.position.location_x
) ** 2 + (self.waypoint.location_y - report.position.location_y) ** 2
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using your defined function with report.position as the input


elif distance_from_waypoint < self.acceptance_radius**2:
smallest_distance = float("inf")
smallest_distance_pad = None
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 you handle the case where the landing pad is none below - in this case you would return the default command

(smallest_distance_pad.location_y - self.waypoint.location_y),
)
if smallest_distance_pad is None:
command = commands.Command.create_null_command()
Copy link
Contributor

Choose a reason for hiding this comment

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

we'd prefer just returning (early exit is generally better than nested if statements

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