Ulvetanna
commented
3 months ago Perfect ! I will review in the morning with fresh eyes :)
Closed gecoombs closed 3 months ago
Ulvetanna
commented
3 months ago Perfect ! I will review in the morning with fresh eyes :)
Ulvetanna
commented
3 months ago @gecoombs just to double check the failure of the tests outlined below are as expected
FAILED test_routes_dijkstra.py::test_fuel[./example_routes/dijkstra/fuel/gaussian_random_field.json] FAILED test_routes_dijkstra.py::test_fuel[./example_routes/dijkstra/fuel/gaussian_random_field_waypointsplitting.json] FAILED test_routes_dijkstra.py::test_fuel[./example_routes/dijkstra/time/gaussian_random_field.json] FAILED test_routes_dijkstra.py::test_fuel[./example_routes/dijkstra/time/gaussian_random_field_waypointsplitting.json]
hjabbot
commented
3 months ago @Ulvetanna @ayat-khairy
Running into this indexing issue when running the refactor on the pipeline configs. Any ideas?
Image of the situation from the main branch which runs correctly
@gecoombs thinks it's something to do with Halley not being accessible. The waypoints are Falklands -> everything.
Traceback (most recent call last):
File "/home/habbot/Documents/Work/BAS/repos/sdadt/venv_dev/bin/optimise_routes", line 8, in <module>
sys.exit(optimise_routes_cli())
^^^^^^^^^^^^^^^^^^^^^
File "/home/habbot/Documents/Work/BAS/repos/sdadt/PolarRoute/polar_route/utils.py", line 238, in wrapper
res = func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "/home/habbot/Documents/Work/BAS/repos/sdadt/PolarRoute/polar_route/cli.py", line 158, in optimise_routes_cli
dijkstra_routes = rp.compute_routes(args.waypoints.name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/habbot/Documents/Work/BAS/repos/sdadt/PolarRoute/polar_route/route_planner/route_planner.py", line 541, in compute_routes
self._dijkstra(wp, end_wps)
File "/home/habbot/Documents/Work/BAS/repos/sdadt/PolarRoute/polar_route/route_planner/route_planner.py", line 451, in _dijkstra
consider_neighbours(wp, min_obj_indx)
File "/home/habbot/Documents/Work/BAS/repos/sdadt/PolarRoute/polar_route/route_planner/route_planner.py", line 432, in consider_neighbours
neighbour_map = self.env_mesh.neighbour_graph.get_neighbour_map(_id) # neighbours and cases for node _id
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/habbot/Documents/Work/BAS/repos/sdadt/MeshiPhi/meshiphi/mesh_generation/neighbour_graph.py", line 482, in get_neighbour_map
return self.neighbour_graph [_id]
~~~~~~~~~~~~~~~~~~~~~^^^^^
KeyError: -1
^ That last post now appears to be fixed, however now we're running into another issue where only one path was created out of the potential 5.
Out of them, Halley was the only inaccesible waypoint, but was also second on the list, so perhaps the routing is stopping when it's getting to an inaccessible waypoint and not attempting to do the others?
EDIT: I tried reordering the waypoints in the csv and it still only computes 1 route
gecoombs
commented
3 months ago The above issues should be fixed with the latest changes. I've also created a simplified example to test these cases (see screenshot) which I will add as an additional regression test, unless anyone objects?
hjabbot
commented
3 months ago Ok, results are in for the pipeline config test I did. All seems pretty minor, but worth looking over @SamuelHall700 @Ulvetanna @foxm1
Added keys in refactor mesh that aren't in main:
main isn't producing it at the moment, so... Guess that's a bug fix?Different values:
JSON Formatting:
The extra keys are a minor issue, and should probably stay in as they seem potentially useful down the line?
Differing values are a bit more of a concern. All the values are pretty close to the values from main, and the regression tests pass, so they should be within the tolerance we specify with the reg tests. Personally, I think it's not worth pursuing and the refactor is good enough.
I think the NaN->null in the JSON output is probably a good thing, as NaN isn't valid for JSON anyway.
gecoombs
commented
3 months ago The new config field "unit_shipspeed" is expected to be there. It's a new config parameter that allows input speed units other than km/hr to be specified (e.g. knots).
The difference in waypoint location is due to how we handle the waypoint correction. In the main code when we move a waypoint from an inaccessible cell this change is carried through into the waypoints saved in the json output whereas in the refactor the original input waypoints are saved in the output. I think it makes sense to record the waypoints actually supplied to generate the routes, the minor changes due to waypoint correction will still be present in the route coordinates.
It should be a simple enough change to revert though, if the general opinion is to keep things as they are.
gecoombs
commented
3 months ago I've added a new regression test multi_waypoint_blocked for both the Dijkstra and smoothing to cover the cases highlighted by @hjabbot 's testing on this PR that weren't caught by the existing regression tests:
cellboxes array within the input meshI have also rerun all the routing tests with the changes made since this PR was submitted, results here: route_test_dump_130824.txt
Dijkstra case:
Smoothed case:
PolarRoute Pull Request
Date: 06/08/2024 Version Number: 0.6.1
Description of change
A refactor of the route planner code that implements new classes for routes, route segments and waypoints and removes most usage of pandas in the Dijkstra stage of route optimisation. There are also many minor changes to improve clarity and compliance with PEP-8 guidelines.
Fixes #240
Testing
There are still 10 tests failing with the current version of the code (see attached log). These are all failures of the
test_fueltest in cases where wind resistance is included and represent small discrepancies in the calculated fuel costs (<1% difference). The great circle tests have also been updated as these produced different routes between versions but with identical fuel and time costs. This was due to the presence of non-unique solutions in the zero field case.Files changed:
route_test_dump.txt
Checklist
0.6.xbranch (DO NOT SUBMIT A PR TO MAIN)