Search pattern #168

nsmartinx · 2024-03-01T22:39:51Z

No description provided.

mgupta27

Reviewed.

modules/decision/search_pattern.py

mgupta27 · 2024-03-02T20:34:03Z

modules/decision/search_pattern.py

+        self.target_posy = self.search_origin.east + self.relative_target_posy
+
+    def distance_to_target_squared(self,
+                                   current_position: odometry_and_time.OdometryAndTime


Nit: Add a comma at the end here

TongguangZhang · 2024-03-06T15:15:04Z

modules/decision/search_pattern.py

+            Camera's field of view, measured in degrees. Use the smallest measurement available.
+            Is essentially assumed to be a circle


does the field of view refer to the the angle between the line to the furthest point the camera can see and straight down, or double that angle (I'm assuming it's double the angle)? You don't have to change anything here, just remember to put this in the documentation

TongguangZhang · 2024-03-06T15:16:04Z

modules/decision/search_pattern.py

+        self.search_radius = 0
+        self.acceptable_variance_squared = acceptable_variance_squared
+
+        # Store the origin of the search and 


Nit: comments should be complete sentences (with periods at the end)

TongguangZhang · 2024-03-06T15:33:37Z

modules/decision/search_pattern.py

+            self.max_pos_in_ring = (ceil(self.search_radius * 2 * pi / self.search_gap))
+            self.angle_between_positions = ((2 * pi) / (self.max_pos_in_ring + 1))


remove extra brackets in these lines

TongguangZhang · 2024-03-06T15:36:27Z

modules/decision/search_pattern.py

+            self.max_pos_in_ring = (ceil(self.search_radius * 2 * pi / self.search_gap))
+            self.angle_between_positions = ((2 * pi) / (self.max_pos_in_ring + 1))


Can you walk me through the math you did here? since the search pattern isn't actually a circle but an inscribed polygon, the circumference self.search_radius * 2 * pi would be bigger than the perimeter of the polygon right? This gives us too many search points - we're going in more than a circle.

TongguangZhang · 2024-03-06T15:38:26Z

modules/decision/search_pattern.py

+    def distance_to_target_squared(self,
+                                   current_position: odometry_and_time.OdometryAndTime


Make this a private static method since it's a helper function and isn't changing the class - update the name to be __distance_to_target_squared (double underscore indicates private method)

TongguangZhang · 2024-03-06T15:44:54Z

modules/decision/search_pattern.py

+                 search_height: float,
+                 search_overlap: float,
+                 current_position: odometry_and_time.OdometryAndTime,
+                 acceptable_variance_squared: float):


variance refers more to a probability/statistics concept, could you rename this to something like distance_square_threshold?

TongguangZhang · 2024-03-06T15:51:39Z

tests/test_search_pattern.py

+        distance = search_maker.distance_to_target_squared(drone_odometry)
+        assert distance <= 0.1 


set up tests in the format

setup - get the objects you need and set the expected value, hard code the expected value with expected = True

run the functions to get the actual value - distance = search_maker ..., and set actual = distance <= 0.1

assertions - assert that actual == expected

this is just an example for this test case, do this for the other test cases too

TongguangZhang

Reviewed

TongguangZhang · 2024-05-03T15:17:15Z

modules/decision/search_pattern.py

+from .. import decision_command
+from .. import odometry_and_time
+from math import tan, pi, ceil


follow this import convention https://uwarg-docs.atlassian.net/wiki/spaces/CV/pages/2253226033/Python+Style+Convention#Imports
(python default modules first, then installed modules, then local modules)

TongguangZhang · 2024-05-03T15:20:22Z

modules/decision/search_pattern.py

+        self.distance_squared_threshold = distance_squared_threshold
+        self.small_adjustment = small_adjustment
+
+        # The search will be centred around wherever it is initialized
+        self.search_origin_x = current_position_x
+        self.search_origin_y = current_position_y
+
+        # Drones current target
+        self.target_posx = self.search_origin_x
+        self.target_posy = self.search_origin_y
+
+        # Initialize the drone to the first position in the search pattern
+        self.current_square = 1
+        self.current_side_in_square = 0
+        self.current_pos_on_side = -1
+
+        # Calculate the gap between positions search_gap_width is the left/right distance
+        # search_gap_depth is the forwards/backwards distance
+        self.search_width = 2 * search_height * tan((camera_fov_sideways * pi / 180) / 2)
+        self.search_depth = 2 * search_height * tan((camera_fov_forwards * pi / 180) / 2)
+        self.search_gap_width = self.search_width * (1 - search_overlap)
+        self.search_gap_depth = self.search_depth * (1 - search_overlap)


make sure you denote these as private variables by adding __ (double underscore) to the start of each variable

TongguangZhang · 2024-05-03T15:24:14Z

modules/decision/search_pattern.py

+                 distance_squared_threshold: float,
+                 small_adjustment: float):
+
+        # Store values to be used later


Can we have a more descriptive comment here? maybe "Set local constants"

TongguangZhang · 2024-05-03T15:37:34Z

modules/decision/search_pattern.py

+        adjustment = 0
+        if self.search_gap_depth < self.search_width:
+            adjustment = (self.search_gap_width - self.search_gap_depth) / 2


this seems like we're using the adjustment to decrease the square size to just depth if it's less than the width - why don't we use the min between depth and width every time?

TongguangZhang · 2024-05-03T15:39:09Z

modules/decision/search_pattern.py

+        self.calculate_square_corners()
+        self.calculate_side_of_square()
+
+    def calculate_square_corners(self):


make sure to set this as a private function by adding double underscore to the beginning

TongguangZhang · 2024-05-03T15:39:17Z

modules/decision/search_pattern.py

+        square_size = (min(self.search_gap_depth, self.search_gap_width)
+                 + (self.current_square - 1) * self.search_gap_width)


why do we use the min of depth and width for the first square but always width for the next squares?

TongguangZhang · 2024-05-03T15:39:23Z

modules/decision/search_pattern.py

+            (self.search_origin_x - square_size, self.search_origin_y - square_size - adjustment),
+        ]
+
+    def calculate_side_of_square(self):


make sure to set this as a private function by adding double underscore to the beginning

TongguangZhang · 2024-05-03T15:47:17Z

modules/decision/search_pattern.py

+        if self.moving_horizontally:
+            side_length = next_corner[0] - self.current_corner[0]
+        else:
+            side_length = next_corner[1] - self.current_corner[1]


is the side length not a multiple of width (if moving horizontally), or depth (if moving vertically)?

TongguangZhang · 2024-05-03T16:07:23Z

modules/decision/search_pattern.py

+            side_length = next_corner[1] - self.current_corner[1]
+
+        # Calculate the number of stops needed along the current side
+        self.max_pos_on_side = ceil(abs(side_length) / self.search_gap_depth)


the number of stops should just be the multiple right?

TongguangZhang

reviewed

TongguangZhang · 2024-05-27T13:56:46Z

modules/decision/search_pattern.py

+        self.__search_width = (
+            2 * search_height * tan((camera_fov_sideways * pi / 180) / 2)
+        )
+        self.__search_depth = (
+            2 * search_height * tan((camera_fov_forwards * pi / 180) / 2)
+        )
+        self.__search_gap_width = self.__search_width * (1 - search_overlap)
+        self.__search_gap_depth = self.__search_depth * (1 - search_overlap)


make these 1 step or use *=

modules/decision/search_pattern.py

TongguangZhang · 2024-05-27T13:58:25Z

modules/decision/search_pattern.py

+        adjustment = 0
+        if self.__search_gap_depth < self.__search_width:
+            adjustment = (self.__search_gap_width - self.__search_gap_depth) / 2


make this an if else, with else: adjustment = 0

TongguangZhang · 2024-05-27T14:02:56Z

modules/decision/search_pattern.py

+
+        # Calculate the corners based on the offsets and the search origin. Top left corner is moved
+        # right by search_gap_width as the final side of the square will instead cover that part
+        self.__square_corners = [


a few options here to make it clear which is the top left, top right, bottom left, bottom right:

use an object instead that stores the current state (tl, tr, bl, br) with enums

use a dict with an enum class for the keys

have these as class variables with a state variable that's assigned enum values

by enums I mean have an object with constants for the possible states so instead of assigning state = "top left", you would have state = enum.TOP_LEFT

TongguangZhang · 2024-05-27T14:07:39Z

modules/decision/search_pattern.py

+        self.__current_corner = self.__square_corners[self.__current_side_in_square]
+        next_corner = self.__square_corners[(self.__current_side_in_square + 1) % 4]
+
+        # Determine if the drone is moving horizontally or vertically along the current side
+        self.__moving_horizontally = self.__current_side_in_square % 2 == 0


from above - having it hardcoded would make this more readable so we can tell which direction (clockwise vs counter clockwise) we're going

modules/decision/search_pattern.py

TongguangZhang · 2024-05-27T14:22:27Z

modules/decision/search_pattern.py

+        dist_to_move = self.__travel_gap * self.__current_pos_on_side
+        if self.__moving_horizontally:
+            self.__target_posx = self.__current_corner[0] + dist_to_move
+        else:
+            self.__target_posy = self.__current_corner[1] + dist_to_move


I would prefer having using x += self.__travel_gap and y += self.__travel_gap since these should represent the current state of the drone to avoid the multiplications and reassigning x and y

TongguangZhang · 2024-05-27T14:23:52Z

tests/test_search_pattern.py

+        distance_squared_threshold=2.0,
+        small_adjustment=1,


if you use these numbers to initialize every instance of the class, use a constant

TongguangZhang · 2024-05-27T14:27:27Z

tests/test_search_pattern.py

+    yield search_pattern_instance
+
+
+def create_drone_odometry(pos_x, pos_y, height):


use a fixture for this

TongguangZhang · 2024-05-27T14:30:22Z

tests/test_search_pattern.py

+
+
+@pytest.fixture()
+def search_pattern_width_greater_height():


if you call it depth in the class, keep the naming consistent

TongguangZhang

reviewed!

TongguangZhang · 2024-06-03T19:01:42Z

tests/test_search_pattern.py

+    assert move[0] == newPos
+    assert (
+        move[1].get_command_type()
+        == decision_command.DecisionCommand.CommandType.MOVE_TO_ABSOLUTE_POSITION
+    )
+    pos = move[1].get_command_position()
+    assert pytest.approx(target_x, 0.1) == pos[0]
+    assert pytest.approx(target_y, 0.1) == pos[1]
+    assert pytest.approx(target_z, 0.1) == pos[2]


set this up with the testing convention

inputs and expected - expected_move = newPos, etc.

actual values - actual_pos = move[0]

assert actual == expected

TongguangZhang · 2024-06-03T19:02:25Z

tests/test_search_pattern.py

+        """
+        Test first 20 positions of search pattern where drone has reached point before being called
+        """
+        coordinates = [


use expected_coordinates as the name here to follow testing convention and make it clear it is the expected output

TongguangZhang · 2024-06-03T19:03:06Z

tests/test_search_pattern.py

+            odometry = drone_odometry(coordinates[i][0], coordinates[i][1], 100)
+            move = search_pattern_width_greater_depth.continue_search(odometry)


use actual_coordinates or another descriptive name here to make it clear this is the output

…ges in tests to reflect these changes

TongguangZhang

LGTM!

mgupta27 reviewed Mar 2, 2024

View reviewed changes

TongguangZhang reviewed Mar 6, 2024

View reviewed changes

TongguangZhang reviewed May 3, 2024

View reviewed changes

TongguangZhang reviewed May 27, 2024

View reviewed changes

TongguangZhang reviewed Jun 3, 2024

View reviewed changes

nsmartinx added 17 commits June 9, 2024 10:51

v 0.1

d46da09

v0.1

43f4c73

updated

a1de1da

Small changes + added test cases

9f22d4e

Small changes to SearchPatternClass to conform with style guide. Chan…

d1369af

…ges in tests to reflect these changes

Changing to rectangular search pattern v0.1

711fcc1

Square pattern

66fee13

formatting

914fbd9

formatting

f15c261

changed to rectangular spiral pattern

c838c1b

style

0eb4a12

spiral pattern cleaned up

5bf0214

formatting

242ecb3

formatting

acf20d9

added tests

8222515

fixed in accordance with code review

b8aae3e

Minor testing formatting changes

86eb3b5

nsmartinx force-pushed the search_pattern branch from c14b812 to 86eb3b5 Compare June 9, 2024 16:10

fixed style changes

8a9bfbe

TongguangZhang approved these changes Jun 9, 2024

View reviewed changes

nsmartinx merged commit b5cd1d4 into main Jun 9, 2024
1 check passed

nsmartinx deleted the search_pattern branch June 9, 2024 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search pattern #168

Search pattern #168

nsmartinx commented Mar 1, 2024

mgupta27 left a comment

mgupta27 Mar 2, 2024

TongguangZhang Mar 6, 2024

TongguangZhang Mar 6, 2024

TongguangZhang Mar 6, 2024

TongguangZhang Mar 6, 2024

TongguangZhang Mar 6, 2024

TongguangZhang Mar 6, 2024

TongguangZhang Mar 6, 2024

TongguangZhang left a comment

TongguangZhang May 3, 2024

TongguangZhang May 3, 2024

TongguangZhang May 3, 2024

TongguangZhang May 3, 2024

TongguangZhang May 3, 2024

TongguangZhang May 3, 2024

TongguangZhang May 3, 2024

TongguangZhang May 3, 2024

TongguangZhang May 3, 2024

TongguangZhang left a comment

TongguangZhang May 27, 2024

TongguangZhang May 27, 2024

TongguangZhang May 27, 2024

TongguangZhang May 27, 2024

TongguangZhang May 27, 2024

TongguangZhang May 27, 2024

TongguangZhang May 27, 2024

TongguangZhang May 27, 2024

TongguangZhang left a comment

TongguangZhang Jun 3, 2024

TongguangZhang Jun 3, 2024

TongguangZhang Jun 3, 2024

TongguangZhang left a comment

		Camera's field of view, measured in degrees. Use the smallest measurement available.
		Is essentially assumed to be a circle

		self.max_pos_in_ring = (ceil(self.search_radius * 2 * pi / self.search_gap))
		self.angle_between_positions = ((2 * pi) / (self.max_pos_in_ring + 1))

		def distance_to_target_squared(self,
		current_position: odometry_and_time.OdometryAndTime

		distance = search_maker.distance_to_target_squared(drone_odometry)
		assert distance <= 0.1

		square_size = (min(self.search_gap_depth, self.search_gap_width)
		+ (self.current_square - 1) * self.search_gap_width)

		yield search_pattern_instance


		def create_drone_odometry(pos_x, pos_y, height):

		odometry = drone_odometry(coordinates[i][0], coordinates[i][1], 100)
		move = search_pattern_width_greater_depth.continue_search(odometry)

Search pattern #168

Search pattern #168

Conversation

nsmartinx commented Mar 1, 2024

mgupta27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TongguangZhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TongguangZhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TongguangZhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TongguangZhang left a comment

Choose a reason for hiding this comment