Conversation
raghumanimehta
left a comment
There was a problem hiding this comment.
Thanks for the PR @rnar17 !! This task was indeed quite complicated.
I have pointed a couple of changes, and I have also requested a review on coordinate systems given PATH's troubles with it. I am in the process of updating the documentation in code and confluence. I will notify you when that's done.
Please use that to verify the coordinates on this PR. You can start working on the other changes I have requested on this PR.
| return [latlon_to_xy(reference=reference_latlon, latlon=pos) for pos in lat_lon_list] | ||
|
|
||
|
|
||
| def rot_to_rad_per_sec(rot: int) -> float: |
There was a problem hiding this comment.
A lot of Magic number violations. You don't need to make all of them constants, but can you explain some of the calculations you are doing here? Wherever it makes sense to you, please repalce magic numbers with appropriately named constants.
| self.ais_ship = ais_ship | ||
|
|
||
| projected_distance = self._calculate_projected_distance() | ||
| # Calculate ROT in radians per second |
| cog_rad = math.radians(self.ais_ship.cog.heading) | ||
| x, y = cs.latlon_to_xy(self.reference, self.ais_ship.lat_lon) | ||
| projected_distance = self._calculate_projected_distance(cog_rad) | ||
| bow_y = self.length / 2 |
|
|
||
| if projected_distance == PROJ_DISTANCE_NO_COLLISION: | ||
| # If no collision detected, create small collision zone around the boat | ||
| RADIUS_MULTIPLIER = 5 |
There was a problem hiding this comment.
How did you come up with 5?
| radius, resolution=16 | ||
| ) | ||
| raw = self._translate_collision_zone(boat_collision_zone) | ||
| self._raw_collision_zone = raw |
There was a problem hiding this comment.
I would suggest assigning self._raw_collision_zone directly.
| [-collision_zone_width, self.length / 2 + projected_distance], | ||
| [collision_zone_width, self.length / 2 + projected_distance], | ||
| [self.width / 2, -self.length / 2], | ||
| elif abs(rot_rps) < 1e-4: |
There was a problem hiding this comment.
How did you come up with this threshold? + Magic number
| [self.width / 2, -self.length / 2], | ||
| elif abs(rot_rps) < 1e-4: | ||
| # Straight line | ||
| collision_zone_width = projected_distance * COLLISION_ZONE_STRETCH_FACTOR * self.width |
There was a problem hiding this comment.
This code is same as earlier, right?
| if abs(rot) == 127: | ||
| rot_dpm = 10.0 | ||
| else: | ||
| rot_dpm = (abs(rot) / 4.733) ** 2 |
There was a problem hiding this comment.
Question: why not remove the abs? It seems that we are first computing the magnitude then multiplying the direction on line 264 (correct me if i am wrong). Doesn't it make sense to just do
| rot_dpm = (abs(rot) / 4.733) ** 2 | |
| rot_dpm = (rot / 4.733) ** 2 |
This now encodes direction directly. I am not sure if this would produce the correct result, please verify this change through testing.
| prepared.prep(self.collision_zone) | ||
| else: | ||
| # Turning motion | ||
| R = speed_kmps / abs(rot_rps) |
There was a problem hiding this comment.
Can you add the units of R to the variable name please?
There was a problem hiding this comment.
This is how much the boat translates per radian right?
| turn_angle = rot_rps * dt | ||
|
|
||
| # Center of rotation in world frame | ||
| cx = x - R * math.sin(cog_rad) * math.copysign(1, rot_rps) |
There was a problem hiding this comment.
I haven't verified this but, could you please check if cog is in the right coordinate system for our applicaiton? Remember, our coordinate systems were a mess and this file was written before we discovered the mess. Please take a look whenever you have time.
Please note the documentaiton might be wrong in places; i appologize for that. I am in the process of fixing it.
There was a problem hiding this comment.
I will also point out that i had discovered that math.sin() and other trigonometric functions might need to be flipped to be used correctly. Please look into that as well.
There was a problem hiding this comment.
Also, can you please explain the math here. Especially why does cx = x - R ... whereas cy = y + R...? That is, why is there a discrepency in the signs of those calculations. It is not immediately obvious to me.
Description
Verification