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

…ges in tests to reflect these changes

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

nsmartinx added 3 commits February 21, 2024 23:53

v 0.1

55889bf

v0.1

6375ae6

updated

17e5663

mgupta27 reviewed Mar 2, 2024

View reviewed changes

Small changes + added test cases

d0eb89e

TongguangZhang reviewed Mar 6, 2024

View reviewed changes

nsmartinx added 5 commits March 6, 2024 11:48

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

c6ec6f0

…ges in tests to reflect these changes

Changing to rectangular search pattern v0.1

44104bb

Square pattern

eb1e9ed

formatting

6049bcf

formatting

ca0e2ce

TongguangZhang reviewed May 3, 2024

View reviewed changes

nsmartinx added 6 commits May 4, 2024 17:29

changed to rectangular spiral pattern

a3dd0b2

style

abd8376

spiral pattern cleaned up

e8c935d

formatting

5193dfd

formatting

fba0a1b

added tests

24f40a7

TongguangZhang reviewed May 27, 2024

View reviewed changes

fixed in accordance with code review

601c8b7

TongguangZhang reviewed Jun 3, 2024

View reviewed changes

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

		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

Are you sure you want to change the base?

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