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

New fork, only committing 3 files, pass all tests and linters #92

Closed
wants to merge 4 commits into from

Conversation

Mmoyv27
Copy link

@Mmoyv27 Mmoyv27 commented Sep 11, 2024

I deleted the old fork and redid all the setup, so this should work.

Only committed 3 files, that pass the tests

Copy link

@ashum68 ashum68 left a comment

Choose a reason for hiding this comment

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

reviewed


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

Choose a reason for hiding this comment

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

get the confidence value using the conf parameter


# 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

Choose a reason for hiding this comment

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

use the detach method to make a copy of the xyxy boxes


# Loop over the boxes list and create a list of bounding boxes
bounding_boxes = []

# Use a range-based for-loop based on number of elements
for i in range(boxes_cpu.shape[0]):
Copy link

Choose a reason for hiding this comment

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

iterate over the list objects instead of the index

"""
closest_pad = None
# Setting default to maximum integer value for easy troubleshooting
min_distance = sys.maxsize
Copy link

@ashum68 ashum68 Sep 12, 2024

Choose a reason for hiding this comment

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

use float('inf') instead to represent infinity.

min_distance = distance
closest_pad = lander

return closest_pad
Copy link

Choose a reason for hiding this comment

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

what if there are no pads? i.e. len(landing_pad_locations) == 0. after handling this, make sure the return type annotation is changed to reflect this.

squared_distance_to_waypoint = (distance_horizontal**2) + (distance_vertical**2)

# Compare shortest distance to acceptance radius w/o square roots for computational efficiency
if squared_distance_to_waypoint <= self.acceptance_radius**2:
Copy link

Choose a reason for hiding this comment

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

nice optimization trick here by not taking the square root. good job!

@@ -38,7 +38,7 @@ class DetectLandingPad:
__MODEL_NAME = "best-2n.pt"

@classmethod
def create(cls, model_directory: pathlib.Path):
def create(cls, model_directory: pathlib.Path) -> tuple:
Copy link

@ashum68 ashum68 Sep 12, 2024

Choose a reason for hiding this comment

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

use this format for tuple type annotations: "tuple[Object1, Object2]" (include the quotations)

@@ -70,6 +120,41 @@ def run(

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

if not self.arrived:
current_position = report.position
Copy link

@ashum68 ashum68 Sep 12, 2024

Choose a reason for hiding this comment

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

why is report.position assigned to this variable? seems like this variable is not needed since report.position is directly referenced later in the code.


# Repeating process but for closest landing pad, after reaching waypoint
if self.arrived:
closest_pad = self.find_closest_landing_pad(
Copy link

Choose a reason for hiding this comment

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

what if closest_pad is None?

Copy link

@ashum68 ashum68 left a comment

Choose a reason for hiding this comment

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

LGTM

@maxlou05 maxlou05 closed this Sep 13, 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.

3 participants