-
Notifications
You must be signed in to change notification settings - Fork 0
Calculates current heading #88
base: main
Are you sure you want to change the base?
Conversation
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 start! I left a few comments 👍
math.cos(current_waypoint.latitude), | ||
(next_waypoint.latitude-current_waypoint.latitude) | ||
) | ||
return heading |
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.
Would it be possible to add some tests for this function?
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.
For the tests, should I create a new test_node_navigate.py file and manually set the value of self.gps to calculate the desired heading?
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 think this would be the best way to test this:
- Create a helper function to
get_desired_heading()
that accepts "simple" arguments like waypoints or lists of waypoints, rather than "complicated" arguments likeself
(which is a whole sailbot object).get_desired_heading()
will basically just call the helper function. - Create
test_node_navigate.py
like you suggested, and just test the helper function. I'd suggest using simple tests like when the desired heading is$0 \degree$ or$90 \degree$ , and maybe some more complicated tests where the "actual" value is computed with geodesic.inv() as well.
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.
Hi Krithik, I left a few comments! We might need to add some more complexity here depending on if we need to determine which local waypoint we're going towards. Jay might be working on this tho so we can talk to him tomorrow.
@@ -175,8 +178,22 @@ def get_desired_heading(self) -> float: | |||
self.gps, self.ais_ships, self.global_path, self.filtered_wind_sensor, self.planner | |||
) | |||
|
|||
# TODO: create function to compute the heading from current position to next local waypoint | |||
return 0.0 | |||
current_waypoint = self.gps.lat_lon |
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 believe self.gps.lat_lon
is the current GPS position of the boat, which is generally not one of the waypoints. I think Jay might be working on code to determine the next local waypoint, but we can sync up with him tomorrow
@@ -175,8 +178,22 @@ def get_desired_heading(self) -> float: | |||
self.gps, self.ais_ships, self.global_path, self.filtered_wind_sensor, self.planner |
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 about line 175:
The function description says we should return something that violates the heading convention in the case of an error, but the heading convention allows
Can you please change the return value on line 175 to something outside those bounds? I'd suggest 404 since that's a common error number for websites haha
break | ||
if next_waypoint is None: | ||
return 0.0 | ||
heading = GEODESIC.inv( |
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 think GEODESIC.inv()
has multiple returns (documentation), and I think you only want the forward azimuth (image at bottom)
I believe Patrick is a fan of this kind of syntax for this situation: _, value, _ = myfunc()
, where myfunc()
returns a touple:
def myfunc():
return (1, 2, 3)
In this example, print(value)
outputs 2.
math.cos(current_waypoint.latitude), | ||
(next_waypoint.latitude-current_waypoint.latitude) | ||
) | ||
return heading |
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 think this would be the best way to test this:
- Create a helper function to
get_desired_heading()
that accepts "simple" arguments like waypoints or lists of waypoints, rather than "complicated" arguments likeself
(which is a whole sailbot object).get_desired_heading()
will basically just call the helper function. - Create
test_node_navigate.py
like you suggested, and just test the helper function. I'd suggest using simple tests like when the desired heading is$0 \degree$ or$90 \degree$ , and maybe some more complicated tests where the "actual" value is computed with geodesic.inv() as well.
Description
Verification
Resources