Skip to content

Conversation

@surgura
Copy link
Contributor

@surgura surgura commented Jun 4, 2024

  • Move all instruments out of sailship in to their own module
  • Refactor most of sailship
    -> tests for instruments are incomplete and their actual results are most likely incorrect. This will be the next step.

@surgura surgura requested review from ammedd and erikvansebille June 4, 2024 09:18
@ammedd
Copy link
Collaborator

ammedd commented Jun 4, 2024

Anything specific you want feedback on?
It feels hard to review tests that are incomplete and likely incorrect.
In general the structure looks good, though little things like that the different particle sets (.e.g _ADCPParticle) are written with an underscore (although I understand from a programming viewpoint) bug me.

Copy link
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me! A few comments and suggestions below

@surgura surgura requested a review from erikvansebille June 5, 2024 14:48
@surgura surgura merged commit fcb36d7 into development Jun 5, 2024
@surgura surgura deleted the seperate_instruments branch June 5, 2024 15:12
surgura added a commit that referenced this pull request Aug 18, 2024
* wip drifters

* drifters instrument. just missing test

* drifter test

* Remove old drifter file and fix errors

* codetools

* outputdf wherever i forgot to use it

* added todo list

* ctd instrument

* CTDtest

* use new ctd instrument in sailship

* fix typo

* codetools

* use new parcels api for particle class

* add ctd to init

* comments

* first try adcp

* adcp test

* use adcp instrument in sailship

* cleanup

* instrument ship s t

* use ship st in sailship

* cleanup

* refactor sailship ctd cast

* cleanup

* large cleanup

* add checks that all drifter and argo floats are deployed

* comments

* docstrings and other codetools fixes

* rename SamplePoint to Spacetime and move them one dir up

* rm todo

* run pydocstyle on tests

* fix some names

* Update virtual_ship/instruments/ctd.py

Co-authored-by: Erik van Sebille <erikvansebille@gmail.com>

* removed comment about ctd

* fix incorrect docstring

* renamed ship_st to ship_underwater_st

* remove sailship create fieldset commented out function

* ship underway st docstring improvement

* minor docstring change

* minor docstring change

---------

Co-authored-by: Erik van Sebille <erikvansebille@gmail.com>
VeckoTheGecko pushed a commit that referenced this pull request Sep 25, 2024
* wip drifters

* drifters instrument. just missing test

* drifter test

* Remove old drifter file and fix errors

* codetools

* outputdf wherever i forgot to use it

* added todo list

* ctd instrument

* CTDtest

* use new ctd instrument in sailship

* fix typo

* codetools

* use new parcels api for particle class

* add ctd to init

* comments

* first try adcp

* adcp test

* use adcp instrument in sailship

* cleanup

* instrument ship s t

* use ship st in sailship

* cleanup

* refactor sailship ctd cast

* cleanup

* large cleanup

* add checks that all drifter and argo floats are deployed

* comments

* docstrings and other codetools fixes

* rename SamplePoint to Spacetime and move them one dir up

* rm todo

* run pydocstyle on tests

* fix some names

* Update virtual_ship/instruments/ctd.py

Co-authored-by: Erik van Sebille <erikvansebille@gmail.com>

* removed comment about ctd

* fix incorrect docstring

* renamed ship_st to ship_underwater_st

* remove sailship create fieldset commented out function

* ship underway st docstring improvement

* minor docstring change

* minor docstring change

---------

Co-authored-by: Erik van Sebille <erikvansebille@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants