Basic test #33

surgura · 2024-05-24T13:15:54Z

Created test framework
Added test that runs sail_ship complete. No assertions yet; just to see if it runs.
Seperated Argo simulation as a seperate module in instruments/argo. Created test for it; similarly without assertions.
Added tests to CI. Full code coverage is not generated or uploaded anywhere yet. Just runs the tests and sees how that goes.

…ata directory a parameter instead of hardcoded

…sed through the config object. Create test that runs a very simple cruise

codetools.sh

tests.sh

erikvansebille · 2024-05-27T13:46:01Z

tests/conftest.py

Why is this necessary? We don't have this in parcels. What does this file do?

This is ran for every test. It chanced the working directory of the test to the directory of the test itself. The reason I did this is because the config json is next to the test, and since the working directory was at the project root I had to type the path to the file.

erikvansebille · 2024-05-27T13:48:20Z

tests/sailship_config.json

Not for this PR, but at some point we should reconsider whether we want to keep using json files. We could also move to yaml-files (with the advantage of allowing comments) or another format?

erikvansebille · 2024-05-27T13:51:27Z

tests/test_sailship.py

+from virtual_ship.virtual_ship_configuration import VirtualShipConfiguration
+from virtual_ship.sailship import sailship


In plasticparcels, we've started to move to a API that is more like numpy etc, where we use an abbreviation (vship?) for all functions. E.g.

import virtualship as vship config = vship.Configuration(...) vship.sailship(config)

Should we start doing that here too?

Fine by me. Regarding the API(what is exported where in which modules), I will look at that after I've finished the first cleanup round.

erikvansebille · 2024-05-27T13:54:37Z

virtual_ship/instruments/argo.py

+class _ArgoParticle(JITParticle):
+    cycle_phase = Variable("cycle_phase", dtype=np.int32, initial=0.0)
+    cycle_age = Variable("cycle_age", dtype=np.float32, initial=0.0)
+    drift_age = Variable("drift_age", dtype=np.float32, initial=0.0)
+    salinity = Variable("salinity", initial=np.nan)
+    temperature = Variable("temperature", initial=np.nan)
+    min_depth = Variable("min_depth", dtype=np.float32)
+    max_depth = Variable("max_depth", dtype=np.float32)
+    drift_depth = Variable("drift_depth", dtype=np.float32)
+    vertical_speed = Variable("vertical_speed", dtype=np.float32)
+    cycle_days = Variable("cycle_days", dtype=np.int32)
+    drift_days = Variable("drift_days", dtype=np.int32)


Since Parcels v3.0.2 (Parcels-code/Parcels#1501), you can also make this cleaner as

Suggested change

class _ArgoParticle(JITParticle):

cycle_phase = Variable("cycle_phase", dtype=np.int32, initial=0.0)

cycle_age = Variable("cycle_age", dtype=np.float32, initial=0.0)

drift_age = Variable("drift_age", dtype=np.float32, initial=0.0)

salinity = Variable("salinity", initial=np.nan)

temperature = Variable("temperature", initial=np.nan)

min_depth = Variable("min_depth", dtype=np.float32)

max_depth = Variable("max_depth", dtype=np.float32)

drift_depth = Variable("drift_depth", dtype=np.float32)

vertical_speed = Variable("vertical_speed", dtype=np.float32)

cycle_days = Variable("cycle_days", dtype=np.int32)

drift_days = Variable("drift_days", dtype=np.int32)

class _ArgoParticle = JITParticle.add_variables([

Variable("cycle_phase", dtype=np.int32, initial=0.0),

Variable("cycle_age", dtype=np.float32, initial=0.0),

Variable("drift_age", dtype=np.float32, initial=0.0),

Variable("salinity", initial=np.nan),

Variable("temperature", initial=np.nan),

Variable("min_depth", dtype=np.float32),

Variable("max_depth", dtype=np.float32),

Variable("drift_depth", dtype=np.float32),

Variable("vertical_speed", dtype=np.float32),

Variable("cycle_days", dtype=np.int32),

Variable("drift_days", dtype=np.int32),

])

I am not sure if this is cleaner. If we later introduce a type checker this is probably harder to check than the original class.

It's cleaner because users can easily create bugs/issues when the varname on the left side of the equal sign is not the same as the varname in the Variable object; that is why I want to move away from the varname = Variable(varname) way of declaring. So I would strongly advice to change to the new way of creating a new Particle Class

Removing duplication makes sense, I replaced. Regarding parcels, you could also look at the dataclass and fields API and try do something similar: https://www.studytonight.com/post/python-dataclass-field-function-part-3

virtual_ship/instruments/argo.py

erikvansebille · 2024-05-27T13:57:42Z

virtual_ship/instruments/argo.py

+    )
+
+    # get time when the fieldset ends
+    fieldset_endtime = fieldset.time_origin.fulltime(fieldset.U.grid.time_full[-1])


Why do we run to the end of the fieldset?

We allowed 6 max weeks of floating time, 3 week ship time + 3 weeks after cruise ended, but since they are deployed at different times end times would also have been different if set at 6 weeks, this was our workaround

We can discuss. Right now I attempted to reproduce the original code as much as possible. Maybe we can have an end time for every argo? I assume that if the end time of the argo is later than the end time of the fieldset that is an error?

ammedd

Looks good. Left some comments about documentation and questions

virtual_ship/instruments/argo.py

ammedd · 2024-05-27T13:48:25Z

virtual_ship/instruments/argo.py

+    drift_days: float
+
+
+class _ArgoParticle(JITParticle):


In a next step you will test for JIT and SciPy right?

Yes, the tests will be expanded later

ammedd · 2024-05-27T13:50:02Z

virtual_ship/instruments/argo.py

+    max_depth = Variable("max_depth", dtype=np.float32)
+    drift_depth = Variable("drift_depth", dtype=np.float32)
+    vertical_speed = Variable("vertical_speed", dtype=np.float32)
+    cycle_days = Variable("cycle_days", dtype=np.int32)


I guess I would use float ipv int for flexibility

Do you mean that you want fractional days?

ammedd · 2024-05-27T13:51:12Z

virtual_ship/instruments/argo.py

+    drift_age = Variable("drift_age", dtype=np.float32, initial=0.0)
+    salinity = Variable("salinity", initial=np.nan)
+    temperature = Variable("temperature", initial=np.nan)
+    min_depth = Variable("min_depth", dtype=np.float32)


Why are you being so explicit about defining these instead of letting parcels create them?

I am not sure what you mean?

ammedd · 2024-05-27T13:53:54Z

virtual_ship/instruments/argo.py

+    """
+    lon = [argo.location.lon for argo in argos]
+    lat = [argo.location.lat for argo in argos]
+    time = [argo.deployment_time for argo in argos]


Is there any check if Lon, lat and time have the same length? Otherwise it fails? or cycles through the shorter?

All three loops cycle over the the same argo float array

ammedd · 2024-05-27T13:56:21Z

virtual_ship/instruments/argo.py

+    )
+
+    # get time when the fieldset ends
+    fieldset_endtime = fieldset.time_origin.fulltime(fieldset.U.grid.time_full[-1])


In the future it would be good to have multiple reasons to end the simulation, not just the end of the fieldset, but also a predefined time, or ...

ammedd · 2024-05-27T14:01:51Z

virtual_ship/instruments/location.py

@@ -0,0 +1,29 @@
+"""Location class."""


It is not clear to me yet how you will use this class. Can you explain? or is this for later?

The discription for the class is in the docstring of the class. I did not want to repeat the description, so this docstring is fairly useless. It is used in the instruments to define a location on the globe

ammedd · 2024-05-27T14:10:19Z

virtual_ship/instruments/argo.py

+    )
+
+    # get time when the fieldset ends
+    fieldset_endtime = fieldset.time_origin.fulltime(fieldset.U.grid.time_full[-1])


We allowed 6 max weeks of floating time, 3 week ship time + 3 weeks after cruise ended, but since they are deployed at different times end times would also have been different if set at 6 weeks, this was our workaround

ammedd · 2024-05-27T14:12:04Z

virtual_ship/sailship.py

This file changed too much from my file for me to oversee the differences. I'll leave it to Erik or until we discuss

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

erikvansebille

Approved, except for changing the custom Particle Class (see comment above about the ArgoParticle)

erikvansebille · 2024-05-28T18:01:33Z

.github/workflows/codetools.yml

+      - name: install
+        run: pip install ".[dev]"
+      - name: run_tests
+        run: pytest --cov=virtual_ship tests


Add extra verbose/debug output to pytest

Suggested change

run: pytest --cov=virtual_ship tests

run: pytest -v --cov=virtual_ship tests

erikvansebille · 2024-05-28T18:02:30Z

tests.sh

+
+# Runs the tests and creates a code coverage report.
+
+pytest --cov=virtual_ship --cov-report=html tests


Also here add extra verbose/debug output

Suggested change

pytest --cov=virtual_ship --cov-report=html tests

pytest -v --cov=virtual_ship --cov-report=html tests

erikvansebille · 2024-05-28T18:06:29Z

virtual_ship/instruments/argo.py

+class _ArgoParticle(JITParticle):
+    cycle_phase = Variable("cycle_phase", dtype=np.int32, initial=0.0)
+    cycle_age = Variable("cycle_age", dtype=np.float32, initial=0.0)
+    drift_age = Variable("drift_age", dtype=np.float32, initial=0.0)
+    salinity = Variable("salinity", initial=np.nan)
+    temperature = Variable("temperature", initial=np.nan)
+    min_depth = Variable("min_depth", dtype=np.float32)
+    max_depth = Variable("max_depth", dtype=np.float32)
+    drift_depth = Variable("drift_depth", dtype=np.float32)
+    vertical_speed = Variable("vertical_speed", dtype=np.float32)
+    cycle_days = Variable("cycle_days", dtype=np.int32)
+    drift_days = Variable("drift_days", dtype=np.int32)


It's cleaner because users can easily create bugs/issues when the varname on the left side of the equal sign is not the same as the varname in the Variable object; that is why I want to move away from the varname = Variable(varname) way of declaring. So I would strongly advice to change to the new way of creating a new Particle Class

Created test framework Added test that runs sail_ship complete. No assertions yet; just to see if it runs. Seperated Argo simulation as a seperate module in instruments/argo. Created test for it; similarly without assertions. Added tests to CI. Full code coverage is not generated or uploaded anywhere yet. Just runs the tests and sees how that goes.

surgura added 11 commits April 30, 2024 14:43

Move deployments and postprocessing into sailship function and make d…

16159dc

…ata directory a parameter instead of hardcoded

ignore .vscode/

a41afab

ignore .vscode/

dbb7f0d

Crudely move fieldset creation out of sailship and onto the user, pas…

34a44a7

…sed through the config object. Create test that runs a very simple cruise

Create sensors module, starting with argos. create basic test for argos.

56f44b5

Use argo sensor in sailship function

fd6f84e

rename sensors to instruments

17948e1

rename sensors to instruments

0a6ab7a

Resolved codetools errors

c2bb72f

Make constants individual parameters for argos

528d1b3

update sailship to use new argo function

02ee54d

surgura assigned erikvansebille and ammedd May 24, 2024

surgura changed the base branch from main to development May 24, 2024 13:16

whitespace

a694733

surgura commented May 24, 2024

View reviewed changes

codetools.sh Show resolved Hide resolved

surgura added 7 commits May 24, 2024 15:21

whitespace

635110e

comment

72a83cd

Add running tests with codecov

ad173e2

Add missing dependency

89f88a6

Add missing dependencies

49c7d50

fix dependency?

e566bdb

fix dependency?

9ad6008

surgura marked this pull request as ready for review May 24, 2024 14:16

erikvansebille reviewed May 27, 2024

View reviewed changes

ammedd reviewed May 27, 2024

View reviewed changes

surgura and others added 4 commits May 27, 2024 17:05

Add missing newline

c4d4555

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

Comments update

87b040f

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

Docstring update

3e42563

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

Rename argos to argo floats

01ffb83

surgura added 3 commits May 28, 2024 14:43

Create proper init.py

1e1267f

make outputdf a parameters for argos

3b65eca

Minor docstring update

36ead1b

surgura requested review from ammedd and erikvansebille May 28, 2024 14:25

surgura added 2 commits May 28, 2024 16:34

minor variable renaming

5186d7e

Run codetools on tests

8bb7ee2

erikvansebille approved these changes May 28, 2024

View reviewed changes

Use new parcels particle api

1cf2082

surgura force-pushed the development branch from e22c580 to 6958101 Compare May 29, 2024 15:02

surgura merged commit d6307b9 into development May 29, 2024

surgura deleted the basic_test branch May 29, 2024 15:03

		from virtual_ship.virtual_ship_configuration import VirtualShipConfiguration
		from virtual_ship.sailship import sailship

	run: pytest --cov=virtual_ship tests
	run: pytest -v --cov=virtual_ship tests


		# Runs the tests and creates a code coverage report.

		pytest --cov=virtual_ship --cov-report=html tests

	pytest --cov=virtual_ship --cov-report=html tests
	pytest -v --cov=virtual_ship --cov-report=html tests

Basic test #33

Basic test #33

Uh oh!

Conversation

surgura commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ammedd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikvansebille left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

surgura commented May 24, 2024 •

edited

Loading