PolarRoute - New Vehicles - BoatyMcBoatFace, TwinOtter & Windracers

[Ulvetanna]

PolarRoute Pull Request Template

Date:
Version Number:

Description of change

Fixes # (issue)

Testing

To ensure that the functionality of the PolarRoute codebase remains consistent throughout the development cycle a testing strategy has been developed, which can be viewed in the document test/testing_strategy.md. This includes a collection of test files which should be run according to which part of the codebase has been altered in a pull request. Please consult the testing strategy to determine which tests need to be run.

• [x] My changes have not altered any of the files listed in the testing strategy

• [ ] My changes result in all required regression tests passing without the need to update test files.

list which files have been altered and include a pytest.txt file for each of the tests required to be run

The files which have been changed during this PR can be listed using the command

git diff --name-only 0.3.x

• [ ] My changes require one or more test files to be updated for all regression tests to pass.

include pytest.txt file showing which tests fail.
include reasoning as to why your changes cause these tests to fail.

Should these changes be valid, relevant test files should be updated.
include pytest.txt file of test passing after test files have been updated.

Checklist

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

[Ulvetanna]

All changes have been resolved above @gecoombs can you take a second look just incase.

[hjabbot]

Generally looks good and nice to see the extension of the vessel performance models with new types of vehicle. Unless there's a good reason to keep them separate I think you should merge the abstract classes for the planes/UAVs to avoid duplication of code. You can add any specific differences in the implemented classes for each vehicle. Otherwise just a few comments on naming/formatting/docstrings etc.

I concur, and on that point... is there a reason that none of the functions are defined in the abstract_vessel.py @gecoombs ? Is it set up to be an interface like dataloader_interface.py? e.g. land() defined in abstract_ship() is generic and reads from the config, I don't see why this would be different for any other vessel, and hence why it wouldn't be in abstract_vessel.py

[gecoombs]

Generally looks good and nice to see the extension of the vessel performance models with new types of vehicle. Unless there's a good reason to keep them separate I think you should merge the abstract classes for the planes/UAVs to avoid duplication of code. You can add any specific differences in the implemented classes for each vehicle. Otherwise just a few comments on naming/formatting/docstrings etc.

I concur, and on that point... is there a reason that none of the functions are defined in the abstract_vessel.py @gecoombs ? Is it set up to be an interface like dataloader_interface.py? e.g. land() defined in abstract_ship() is generic and reads from the config, I don't see why this would be different for any other vessel, and hence why it wouldn't be in abstract_vessel.py

It was quite a while ago now but yeah I think I based the structure on what had been done as part of the previous refactor for the dataloaders etc. although I'm open to the suggestion that this interface structure isn't necessarily required or could be renamed. Any change for that purpose is probably outside the scope of this pull request though.