-
Notifications
You must be signed in to change notification settings - Fork 266
Arushi's pull request #149
base: main
Are you sure you want to change the base?
Conversation
maxlou05
left a comment
There was a problem hiding this 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include the confidence value in the image as indicated above, set it using a kwarg like in the predict() method
| # when the drone is halted but not at the destination | ||
| if ( | ||
| report.status == drone_status.DroneStatus.HALTED | ||
| and report.destination != self.waypoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using direct equality comparison of floating point numbers (which are not very accurate and may not give you the result you want), you should use the self.acceptance_radius as a guide.
| and report.destination != self.waypoint | ||
| ): | ||
| command = commands.Command.create_set_relative_destination_command( | ||
| self.waypoint.location_x, self.waypoint.location_y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a relative distance command, so your own position matters
| # when the drone is at the destination and is halted | ||
| if ( | ||
| report.status == drone_status.DroneStatus.HALTED | ||
| and report.destination == self.waypoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, use acceptance radius rather than equality
| # ↑ BOOTCAMPERS MODIFY ABOVE THIS COMMENT ↑ | ||
| # ============ | ||
|
|
||
| def min_dist_squared(self, landing_pads: location.Location) -> float: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should rename your function and its parameters to match the description better. But good job including a doc string!
| if report.status == drone_status.DroneStatus.HALTED: | ||
|
|
||
| # when drone is at the nearest landing pad | ||
| if self.has_sent_landing_command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this variable to something else, as it feels awkward to send a land command after "has_sent_landing_command" is true. Code readability and communication is also important!
| # setting relative destination to designated waypoint | ||
| elif proximity > self.acceptance_radius and not self.has_sent_landing_command: | ||
| command = commands.Command.create_set_relative_destination_command( | ||
| self.waypoint.location_x, self.waypoint.location_y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relative distance command, your own position matters
| self.has_sent_landing_command = True | ||
|
|
||
| # setting relative destination to designated waypoint | ||
| elif proximity > self.acceptance_radius and not self.has_sent_landing_command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
distance squared
| # * device | ||
| # * verbose | ||
| predictions = ... | ||
| # model = ultralytics.YOLO("yolov8n.pt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can delete commented out code since that's what git is for, it keeps track of history for you
maxlou05
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
| # when the drone is at the destination and is halted | ||
| if ( | ||
| report.status == drone_status.DroneStatus.HALTED | ||
| and proximity < self.acceptance_radius |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot squared
| # Plot the annotated image from the Result object | ||
| # Include the confidence value | ||
| image_annotated = ... | ||
| image_annotated = prediction.plot(conf=0.7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a boolean, ie to include it in the image or not
| proximity = (self.waypoint.location_x - report.position.location_x) ** 2 + ( | ||
| self.waypoint.location_y - report.position.location_y | ||
| ) ** 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you are constantly calculating the distance to the waypoint, which is probably not necessary all the time, such as after you reached the waypoint
| new_pad = landing_pads | ||
|
|
||
| command = commands.Command.create_set_relative_destination_command( | ||
| new_pad.location_x - self.waypoint.location_x, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be your own position: if your drone stopped midway to the waypoint, it would overshoot the landing pad by a lot
maxlou05
left a comment
There was a problem hiding this 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(conf=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use True instead of 1
| if new_pad == self.waypoint: | ||
| command = commands.Command.create_set_relative_destination_command( | ||
| self.waypoint.location_x - report.position.location_x, | ||
| self.waypoint.location_y - report.position.location_y, | ||
| ) | ||
| self.send_landing_command = True | ||
| else: | ||
| command = commands.Command.create_set_relative_destination_command( | ||
| new_pad.location_x - report.position.location_x, | ||
| new_pad.location_y - report.position.location_y, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if statement doesn't really accomplish anything, both of them just go to new_pad. I meant your proximity variable is always distance to waypoint, so what happens if has_sent_landing_command is True (you reached the waypoint) but the drone halts and you are no longer close to the waypoint? You only rely on send_landing_command=True to land the drone, so it'll land anywhere it stops. I see that you added a has_reached_waypoint variable, but it is not used anywhere
maxlou05
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
| # when the drone is at the destination and is halted | ||
| if ( | ||
| report.status == drone_status.DroneStatus.HALTED | ||
| and proximity < self.acceptance_radius**2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably put <= just in case that it happens to be equal.
| # calculating location of nearest landing pad | ||
| smallest_dist_x = float("inf") | ||
| smallest_dist_y = float("inf") | ||
| smallest_dist = smallest_dist_x**2 + smallest_dist_y**2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just set it to infinity, as you don't really need to calculate infinity squared
| smallest_dist_x - report.position.location_x, | ||
| smallest_dist_y - report.position.location_y, | ||
| ) | ||
| self.reached_landing_pad = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't really know that the drone has reached the landing pad here, you only know that you told the drone to go towards the landing pad. What if it doesn't get there?
|
|
||
| # if drone halts unexpectedly, neither at the waypoint or at the nearest landing pad | ||
| else: | ||
| command = commands.Command.create_set_relative_destination_command( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you know that you should go to the waypoint and not the landing pad? What if you were en route towards the landing pad?
No description provided.