AMaharaj16/test-conditions-switch-path-669#743
Conversation
…e_navigate to test_local_path.
…nd heading even though old path is most optimal.
…into AMaharaj16/test-conditions-switch-path-669
…est for improved clarity and robustness in testing.
…s OMPLPath parameters but I can't pick waypoints in OMPLPath. Current solution is to move get_cost back into update_if_needed and pass costs into compare_path_costs where total costs (add heading cost) is calculated and cheapest path returned.
…on, will try to test from here now that I know the issues.
…d passing tests to the function. Heading calculations are good, now I need to look into testing path costs.
AMaharaj16
requested review from
FireBoyAJ24,
SPDonaghy,
alberto-escobar,
raghumanimehta and
stevenxu27
as code owners
|
14 files changed... that's not right lol. I stopped coding on this for a few weeks during finals and I think I committed some files I didn't mean to change after I came back. I will look into fixing this. |
raghumanimehta
left a comment
raghumanimehta
left a comment
There was a problem hiding this comment.
Nice work on the PR! Requesting a bunch of refactoring the code before I merge this to the main branch. As soon as you are done with the changes, let me know and move on to the next half of testing.
|
|
||
| metric_old = w_h * heading_diff_old_normalized + w_c * old_cost_normalized | ||
| metric_new = w_h * heading_diff_new_normalized + w_c * new_cost_normalized | ||
| metric_old, metric_new = self.calculate_metric(heading, heading_old_path, heading_new_path, |
There was a problem hiding this comment.
Instead of returning a tuple of old and new, you can make function reusable such that it doesn't care if you pass the old or new path. You can call compute metric on the old_path and new_path separately to get old and new metric respectively.
For example:
new_metric = calculate_metric(heading_new_normalized, heading_old_normalized)
This function right now is very tightly coupled with the caller. Also, I don't like the assymetric behaviour where we are normalizing the cost outside the function but normalizing the heading inside.
…github.com/UBCSailbot/sailbot_workspace into AMaharaj16/test-conditions-switch-path-669
…r than 0.1 degree heading diff from old path.
|
Don't merge just yet, last test fails every 8-10 test runs. Since the function can have fluctuating outputs, testing boundary cases (improvement threshold) will be quite inconsistent. I think we should either remove the last test or change it so it covers another case such as one with >3 waypoints and ensure it passes consistently. What do you think @raghumanimehta? I can finish it from there onwards. Also, I have run all other test cases for |
raghumanimehta
left a comment
raghumanimehta
left a comment
There was a problem hiding this comment.
The task is yet not complete. Merging because, there are notable improvements in update_if_needed
| return True | ||
|
|
||
|
|
||
| def create_mock( |
There was a problem hiding this comment.
Since this function is strictly going to be used for testing, we can safely move this to the test file.
…github.com/UBCSailbot/sailbot_workspace into AMaharaj16/test-conditions-switch-path-669 merge
3 participants
Refactored the
update_if_neededfunction to improve testability by extracting the metric calculation logic (which combines heading difference cost and path cost) into a separate, testable unit calledcalculate_metric. Test suite forcalculate_metricfully passes. No actual computation changes made.Also moved
test_calculate_desired_heading_and_waypoint_indexfromtest_node_navigate.pytotest_local_path.pyand added testing forcalculate_heading_diff.Next, I plan to test
update_if_neededaltogether using a deterministic function that creates OMPLPath mock objects with predetermined paths to enable controlled testing.