Misc Post Refactor Changes

[gecoombs]

PolarRoute Pull Request

Date: 13/08/24
Version Number: 0.6.2

Description of change

Full list of changes:

• Added kml output option for both optimise_routes and extract_routes
• More flexible handling of waypoint csv files
• Tidied up some duplicate functions
• Fixed a bug with battery values for smoothed routes
• Fixed a bug with battery optimisation
• Handle waypoints at the same location and in the same cell
• Fix for waypoint splitting when waypoints have been moved from an inaccessible cell
• New regression test to cover routing between two waypoints in the same cell

Fixes (part of?) #244

Testing

Test logs:
route_test_dump.txt

Files changed:

docs/source/sections/Command_line_interface.rst
polar_route/__init__.py
polar_route/cli.py
polar_route/config_validation/config_validator.py
polar_route/route_planner/crossing_smoothing.py
polar_route/route_planner/route.py
polar_route/route_planner/route_planner.py
polar_route/route_planner/segment.py
polar_route/utils.py
requirements.txt
tests/regression_tests/example_routes/dijkstra/time/single_cell.json
tests/regression_tests/example_routes/smoothed/time/single_cell.json
tests/regression_tests/test_routes_dijkstra.py
tests/regression_tests/test_routes_smoothed.py

Checklist

• My code follows pep8 style guidelines.
• I have commented my code, particularly in hard-to-understand areas.
• I have updated the documentation of the codebase where required.
• My changes generate no new warnings.
• My PR has been made to the 0.6.x branch (DO NOT SUBMIT A PR TO MAIN)

[gecoombs]

I remembered another small issue I wanted to fix in the refactor so have rolled it into this PR: if the user adds a space after the 'X' used to mark Source/Destination in the waypoints csv (or leaves it lower case) then PolarRoute can now handle that without erroring. This involved a change in route_planner.py so I'll rerun the tests.

[gecoombs]

Hopefully this is everything for this PR, could you take a look now @hjabbot and maybe re-run the pipeline configs to check none of this breaks anything there?

There might be an issue with the use of fiona for the kml output on existing installs, as there are actually two different libraries geopandas can use for drivers so depending on the platform it could require a pip install fiona to get this working. I based the implementation here on this post: https://gis.stackexchange.com/a/258370 but I'm open to suggestions for improvement of how this is handled.

[hjabbot]

The reg tests results you provided includes failing tests. Are they done after the most recent changes?

[gecoombs]

The reg tests results you provided includes failing tests. Are they done after the most recent changes?

These are the same failing tests from #286, for fuel values in meshes with wind included. I was still planning to address these separately.

[hjabbot]

Reran the pipeline configs with this code and the outputs match up with 0.6.x, only difference seems to be that the 'fuel' property in the routes has moved down to the bottom, but the values remain the same

Something I noticed running these though is that the refactor is significantly slower than main (2400s vs 1800s for a complicated set of waypoints). It'd be good to see this addressed before merging with main. Shall I raise an issue?

[gecoombs]

Reran the pipeline configs with this code and the outputs match up with 0.6.x, only difference seems to be that the 'fuel' property in the routes has moved down to the bottom, but the values remain the same

Something I noticed running these though is that the refactor is significantly slower than main (2400s vs 1800s for a complicated set of waypoints). It'd be good to see this addressed before merging with main. Shall I raise an issue?

Ok yeah raise it as an issue and I'll look at running some sort of profiling to see where it's losing time.

[SamuelHall700]

Looks good, would be good to see some additional tests done on the new speed of the route planner though!

[gecoombs]

After the last few changes related to #290 the example I was testing has sped up from 265s to 121s (see profiles below). @hjabbot's testing on the pipeline configs shows a smaller but still significant relative time saving (~33%) and in both cases the refactor is now at least as fast as main. I have also rerun the tests and the results are unchanged: route_test_dump_200824.txt

There are probably still optimisations to be found, especially in initialise_dijkstra_graph, but I'm now happy that this doesn't make things any slower.

Before:

After:

Plot of the routes for this example: