-
Notifications
You must be signed in to change notification settings - Fork 199
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
Himank Bootcamp PR #11
base: main
Are you sure you want to change the base?
Conversation
|
||
# Plot the annotated image from the Result object | ||
# Include the confidence value | ||
image_annotated = ... | ||
image_annotated = prediction.plot(pil=True, conf=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.
The expected image is a numpy ndarray, not a PIL object.
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.
According to the Ultralytics documentation, setting img = True
would return numpy.ndarray
type, but that was not the case.
When I tried running pytest with pil=True, conf=True
, all 3 tests passed, whereas running pytest with img=True, conf=True
failed all 3 tests.
The error message was the following: AttributeError: 'bool' object has no attribute 'data'
.
I do not know whether this is a bug or something else.
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.
The img
parameter's type itself is expected to be np.ndarray
. This is so that you can pass in an existing image that Ultralytics will draw over, but this bootcamp does not use it.
If you leave pil=True
, what is image_annotated
object? Do the test output images look fine?
|
||
# Detach the xyxy boxes to make a copy, | ||
# move the copy into CPU space, | ||
# and convert to a numpy array | ||
boxes_cpu = ... | ||
boxes_cpu = prediction.boxes.cpu().xyxy.numpy() |
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.
Use boxes_xyxy
. Also detach before moving to the CPU.
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.
What do you mean by "detach"?
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.
Pytorch tensors have a detach()
method, use that.
if result: | ||
bounding_boxes.append(box) |
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.
In statistics, it is preferred to remove the whole data point if there is an issue (here, the data point is an image). (You do not need to change anything here, just something to think about).
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.
Does that mean removing the entire list of bounding boxes if even one bounding box has an issue?
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.
Correct, an empty list would be returned instead (the entire image is skipped). But you don't need to change anything.
@@ -37,12 +37,67 @@ def __init__(self, waypoint: location.Location, acceptance_radius: float): | |||
# ↓ BOOTCAMPERS MODIFY BELOW THIS COMMENT ↓ | |||
# ============ | |||
|
|||
# Add your own | |||
self.action_dict = dict(zip(("MOVE", "HALT", "LAND"), range(3,6))) | |||
self.origin = location.Location(0,0) |
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.
What if the drone didn't start at (0,0) , but could start anywhere?
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.
Then, in that case, wouldn't we need to keep track of the initial position of the drone?
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.
Yes. Alternatively, don't use the initial position of the drone.
landing_pad_distances = list(map(lambda loc: shortest_distance(loc, given_location), landing_pad_locations)) | ||
landing_pad_hashmap = dict(zip(landing_pad_locations, landing_pad_distances)) | ||
landing_pad_hashmap = tuple(sorted(list(landing_pad_hashmap.items()), key=lambda i: i[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.
This is very convoluted and creates many new objects that will need to be destroyed. Is there a more space efficient way?
Also, sorting is not very efficient. It's possible to get the minimum without sorting the list of landing pad locations.
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.
Fixed this with initializing closest_location
variable with first location in the list and doing a for
loop for the rest of the list to replace the closer location with closest_location
in steadyfall@f42441a.
x_2, y_2 = target_location.location_x, target_location.location_y | ||
x_square = (x_2 - x_1) ** 2 | ||
y_square = (y_2 - y_1) ** 2 | ||
return (x_square + y_square) ** 0.5 |
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.
Square roots are computationally expensive. Is there another way? (You do not need to change anything here, just something to think about.
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.
I do not square root the number and only square root it when required?
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.
That just procrastinating the work, it doesn't eliminate it. What would happen if there was no square root?
if (self.check_if_near_target(landing_pad, report_position) | ||
and (shortest_distance(self.waypoint, report_position) - shortest_distance(self.waypoint, landing_pad) < 0.1) | ||
and ( | ||
(self.check_if_near_target(landing_pad, self.origin) and self.check_if_near_target(self.waypoint, self.origin)) | ||
or (not self.check_if_near_target(report_position, self.origin)) | ||
)): |
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 also a bit complex. What about checking whether the drone has already arrived at the waypoint (e.g. storing some state)?
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 also a bit complex. What about checking whether the drone has already arrived at the waypoint (e.g. storing some state)?
I had tried doing that, but since this method runs in an infinite loop, any data that I try to store goes away.
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.
Declare variables (e.g. self.variable_name
) in __init__()
and use those (e.g. a = self.variable_name
, self.variable_name = 5
). Declare as few or as many as you need.
Those are instance/object variables and will persist with the lifetime of the object.
Returns the relative x and y coordinates for drone to be sent to. | ||
""" | ||
relative_x, relative_y = self.distance_between_waypoint(given_location) | ||
return (relative_x, relative_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.
Remove unneeded parantheses.
@@ -37,12 +37,37 @@ def __init__(self, waypoint: location.Location, acceptance_radius: float): | |||
# ↓ BOOTCAMPERS MODIFY BELOW THIS COMMENT ↓ | |||
# ============ | |||
|
|||
# Add your own | |||
self.action_dict = dict(zip(("MOVE", "HALT", "LAND"), range(3,6))) |
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.
Can use enum
instead.
if action is None: | ||
pass |
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.
Redundant, remove.
|
||
# ============ | ||
# ↑ BOOTCAMPERS MODIFY ABOVE THIS COMMENT ↑ | ||
# ============ | ||
|
||
@staticmethod | ||
def shortest_distance(target_location:location.Location, given_location: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.
Good use of static method!
y_square = (y_2 - y_1) ** 2 | ||
return (x_square + y_square) ** 0.5 | ||
|
||
def relative_coordinates_of_target(self, target_location:location.Location, given_location:location.Location) -> "tuple[float, 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.
This can also be static.
""" | ||
relative_location_x = target_location.location_x - given_location.location_x | ||
relative_location_y = target_location.location_y - given_location.location_y | ||
return (relative_location_x, relative_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.
Remove unneeded parantheses.
Returns the relative x and y coordinates for drone to be sent to. | ||
""" | ||
relative_x, relative_y = self.relative_coordinates_of_target(target_location, given_location) | ||
divider = 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.
Remove this since it doesn't do anything.
""" | ||
Finds out the closest landing pad from the given location by checking out their distances. | ||
""" | ||
closest_location = landing_pad_locations[0] |
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.
What if there were no landing pads? (You do not need to change anything, just something to think about).
distance_from_given_location = DecisionWaypointLandingPads.shortest_distance(landing_pad, given_location) | ||
distance_from_closest_location = DecisionWaypointLandingPads.shortest_distance(closest_location, given_location) |
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.
Calculating the distance repeatedly for the closest landing pad is wasteful, is there a way to avoid this?
I have completed all the 4 given tasks and have created a PR, as per the bootcamp docs