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

Victor Huang Bootcamp #141

Closed
wants to merge 5 commits into from
Closed

Victor Huang Bootcamp #141

wants to merge 5 commits into from

Conversation

vichua2006
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

Comment on lines 75 to 76
elif status == drone_status.DroneStatus.HALTED:
self.been_halted_once = True
Copy link
Contributor

Choose a reason for hiding this comment

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

can you guarantee that the drone is halted at the waypoint? what if the drone halts before that for some reason?

hint: make sure to use the acceptance radius

Comment on lines 77 to 79
command = 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 destination, not absolute, so you can't just use the waypoint's coordinates

Comment on lines 76 to 77
abs(loc1.location_x - loc2.location_x) ** 2
+ abs(loc1.location_y - loc2.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.

do u need abs if ur alr squaring it?

curr_loc: location.Location, loclist: "list[location.Location]"
) -> location.Location:
"""find the closest location in a list of locations"""
min_dist, res = float("inf"), None
Copy link
Contributor

Choose a reason for hiding this comment

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

you should name the var smth more descriptive than res

"""find the closest location in a list of locations"""
min_dist, res = float("inf"), None

for l in loclist:
Copy link
Contributor

Choose a reason for hiding this comment

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

we use snake_case - take a look at the python style guide in confluence for these kinds of conventions

Comment on lines 73 to 80
def find_squared_dist(loc1: location.Location, loc2: location.Location) -> int:
"""helper function that finds the squared distance between two Location instances"""
return (
abs(loc1.location_x - loc2.location_x) ** 2
+ abs(loc1.location_y - loc2.location_y) ** 2
)

def find_closest_location(
Copy link
Contributor

Choose a reason for hiding this comment

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

you should declare these functions outside of the run function (it's ok to go outside the comment guards in this case)

if you declare it like this, each time the function is called, you redeclare these functions, which is unnecessary

Comment on lines 115 to 117
command = 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.

again, relative distance, not absolute

@@ -98,31 +95,35 @@ def run(self, image: np.ndarray) -> "tuple[list[bounding_box.BoundingBox], np.nd
# * conf
# * device
# * verbose
predictions = ...
predictions = self.__model.predict(image, verbose=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

note the other params listed above

Comment on lines 122 to 123
if result is None:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think pass is what you wanna do here - maybe continue?

however, 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

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 70 to 71
status = report.status
position = report.position
Copy link
Contributor

Choose a reason for hiding this comment

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

i usually recommend not doing this - it's more clear when u use the full variable since we know that it's the report's status, position, etc.

Comment on lines 72 to 76
if status == drone_status.DroneStatus.HALTED and (
self.acceptance_radius**2 >= find_squared_dist(self.waypoint, position)
):
command = command.create_land_command()
elif status == drone_status.DroneStatus.HALTED:
Copy link
Contributor

Choose a reason for hiding this comment

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

you check if the drone is halted twice here, maybe do a nested if?



# helper functions
def find_squared_dist(loc1: location.Location, loc2: location.Location) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

you should declare this inside the class (I also recommend making it a static method)

Comment on lines 108 to 113
def find_squared_dist(loc1: location.Location, loc2: location.Location) -> int:
"""helper function that finds the squared distance between two Location instances"""
return (loc1.location_x - loc2.location_x) ** 2 + (loc1.location_y - loc2.location_y) ** 2


def find_closest_location(
Copy link
Contributor

Choose a reason for hiding this comment

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

declare these inside the class

Comment on lines 88 to 89
final_destination = find_closest_location(position, landing_pad_locations)
self.final_destination = final_destination
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe make this one line, you don't really need the second statement

closest_location = loc
min_dist = dist

return closest_location
Copy link
Contributor

Choose a reason for hiding this comment

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

what if the list is empty and closest location is None? make sure to handle that in the run function

@@ -98,31 +95,36 @@ def run(self, image: np.ndarray) -> "tuple[list[bounding_box.BoundingBox], np.nd
# * conf
# * device
# * verbose
predictions = ...
predictions = self.__model.predict(source=image, verbose=False, conf=0.25, device=None)
Copy link
Contributor

Choose a reason for hiding this comment

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

0.25 might be a little low for conf

# Create BoundingBox object and append to list
result, box = bounding_box.BoundingBox.create(raw_box)
# discard image if failed
if result is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

result won't be none, it's a boolean (make sure to check the create function to understand what it returns)

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!

@@ -98,31 +95,36 @@ def run(self, image: np.ndarray) -> "tuple[list[bounding_box.BoundingBox], np.nd
# * conf
# * device
# * verbose
predictions = ...
predictions = self.__model.predict(source=image, verbose=False, conf=0.75, device=None)
Copy link
Member

Choose a reason for hiding this comment

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

Although using default or letting it choose is fine, there is a variable in this bootcamp that indicates which device you should use (You don't have to change anything here, just something to think about)

@maxlou05 maxlou05 closed this Jan 18, 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