-
Notifications
You must be signed in to change notification settings - Fork 1
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
visit water buckets 2 #79
base: main
Are you sure you want to change the base?
Conversation
The waypoint.py file was previously added in a commit, because it was rebased from another branch that used waypoint.py before it was removed from the repo.
from .common.modules.position_global import PositionGlobal | ||
|
||
# Earth radius in meters | ||
EARTH_RADIUS = 6378137 |
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 job breaking this out to a constant
|
||
|
||
def waypoint_distance(point_1: PositionGlobal, point_2: PositionGlobal) -> "tuple[bool, float]": | ||
"""Return the great-circle distance of two points Earth, using Haversine's |
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.
TIL that we don't have an accepted format for docstrings 💀. Nothing to change here but just a quirk we didn't care to catch in prev PRs.
|
||
lat1, lon1, lat2, lon2 = map( | ||
math.radians, | ||
[ |
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.
prefer tuple over lists for uses cases like this
and (True, distance) otherwise, where distance is the great-circle | ||
distance between point_1 and point_2. | ||
""" | ||
# this function only calculates distance for waypoints with the same |
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.
Add this blurb in the docstring
], | ||
) | ||
|
||
dlat = lat2 - lat1 |
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.
don't use contractions in variables names
""" | ||
# this function only calculates distance for waypoints with the same | ||
# altitude | ||
if point_1.altitude != point_2.altitude: |
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 you test how badly it affects accuracy if we ignore altitude difference/take the average, since we add the radius of the Earth I can see the error being insignificant
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.
Move tests to the same file and split with classes
def find_optimal_path( | ||
origin: PositionGlobal, buckets: "list[PositionGlobal]", buckets_at_once: int = 2 | ||
) -> "tuple[bool, list[PositionGlobal]]": | ||
"""Find an optimal itinerary of waypoints, given that we must |
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.
Add a reference to the algo you're using
if not success: | ||
return False, None | ||
|
||
for permutation in itertools.permutations(sorted_buckets): |
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.
If we're doing permutations
why are we sorting the buckets
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.
oh LOL coming from C++ I thought the list must be sorted to iterate over all possible permutations.
|
||
success, path = find_optimal_path(origin, buckets, 2) | ||
assert success | ||
assert path == [ |
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 trust this is the optimal path but it's hard to verify that at a glance. Add a test where the critical path is obvious (3 waypoints, 2 close by one much further; you don't need to base them off real locations)
Find the optimal itinerary, given an origin waypoint (the source of water) and a list of bucket waypoints. We can only visit a fixed number of buckets before we must return to the origin waypoint to fetch more water each time.
Finishes task https://app.asana.com/0/0/1209038151009011/f