Rework wind cost function in ompl objectives#817
Conversation
… docstring to the new functionality.
…asses even if constant values are changed.
raghumanimehta
left a comment
raghumanimehta
left a comment
There was a problem hiding this comment.
Please take a look at the longest comment I left on this review before anything else.
| """Computes a wind alignment cost based on the absolute angle θ between the segment | ||
| bearing and the true wind direction. | ||
|
|
||
| 1) If θ ≤ NO_GO_ZONE or θ ≥ π − NO_GO_ZONE (i.e., within 45 degrees of directly upwind or |
There was a problem hiding this comment.
what is theta? Please define what you mean by theta here.
| return UPWIND_COST_MULTIPLIER * cos_angle | ||
| else: | ||
| return DOWNWIND_COST_MULTIPLIER * abs(cos_angle) | ||
| # NO-GO ZONE |
There was a problem hiding this comment.
This comment, imo, is not required. The reader can understand what this is for by reading the if statement and the docs about the funciton.
| @@ -107,12 +115,13 @@ def wind_direction_cost(s1: cs.XY, s2: cs.XY, tw_direction_rad: float) -> float: | |||
| """ | |||
| segment_true_bearing_rad = cs.get_path_segment_true_bearing(s1, s2, rad=True) | |||
| tw_angle_rad = abs(wcs.get_true_wind_angle(segment_true_bearing_rad, tw_direction_rad)) | |||
There was a problem hiding this comment.
NOT YOUR FAULT.
While reviewing the PR, I realized that the conventions used in PATH are not consistent. Can you please look into it? Or let me know if you want me to look into it.
This funciton's example said that 0 degrees is from the bow. Other parts of the codebase are using 0 degrees as to the bow. This inconsistency is not nice. We should fix it asap. I am not going to review this PR for the time being.
If you do look into this issue, please also change the naming of the winds to our new standard. Eg. tw_speed_kmph_bcrather than wind_speed. There are also some cases where we name it as tw_speed_kmph, however notice the coordinate system is missing. Change these .i.e append bc or gc accordingly .
2 participants
The function
wind_direction_cost()inompl_objectives.pynow correctly returns 1.0 when the segment heading lies within NO_GO_ZONE radians (45 degrees) of directly upwind (0) or directly downwind (π), and applies sin80(2θ) otherwise. Also replaced old test with new fully passing test suite for this improved implementation intest_ompl_objectives.py.The wind cost function looks like this:
