Refactor: instruments have sensors by j-atkins · Pull Request #328 · Parcels-code/virtualship

j-atkins · 2026-04-10T11:44:26Z

Overview

This PR centralises/abstracts instrument sensor/variable sampling logic so that each instrument declares which sensors it carries (e.g. TEMPERATURE, SALINITY, VELOCITY). Users can configure which sensors are active for each instrument in the expedition YAML.

This helps pave the way for easy addition of BGC sensors to the Argo float in a future PR (#234), and consolidation of CTD + CTD_BGC into a single instrument (#260) with a combined sensor allowlist. Also makes it straightforward to add new sensors to any instrument in the future (e.g., #312, #313), and streamlines them process for developers to add new instruments (i.e. #237)

Major changes

sensors.py in instruments/ defines the SensorType class and per-instrument allowlists (so that there is control over which sensors each instrument supports and users cannot configure unsupported sensors).
New SensorConfig pydantic model and sensors field in every instrument config in expedition.py.
New SensorRegistry in utils.py that maps each SensorType to its FieldSet key, Copernicus variable name, category (phys/bgc), and Parcels particle variable name(s).
- Per-instrument parcticle classes are now built dynamically at runtime based on which sensors are active, but the fixed/mechanical variables are still hard-coded in the instrument files, e.g. cycle_phase for Argo Floats.

API change

As mentioned above, the instruments config section of the expedition YAML now has a sensors list field, where users specify which sensors are active. For example, below the CTD is configured to sample TEMPERATURE and SALINITY:

ctd_config:
  stationkeeping_time_minutes: 50
  min_depth_meter: -11
  max_depth_meter: -2000
  sensors:
    - TEMPERATURE
    - SALINITY

If the sensors list is omitted, it default to all valid sensors for that instrument.
By using allowlists for each instrument, it will not allow non-sensical sensor combinations (e.g. BGC sensors on an ADCP).
Will also not allow an empty sensor list, at least one sensor must be active.

Additional change

Argo Float sampling kernels have been separated from the vertical-movement kernel, making it easier to add BGC sensors in a future PR.

Follow-up PRs

The plan CLI tool will need updating to account for sensor configuration options. Currently not broken but doesn't give option to configure sensors. (New issue to be opened)
Docs update to clearly communicate which sensors each instrument accepts, and how to configure them in either the plan tool or the expedition YAML. (New issue to be opened)
Merge CTD + CTD_BGC into a single instrument. (Unify CTD and CTD_BGC to one instrument #260)
Add BGC sampling to Argo Floats (New ARGO_BGC instrument #234)

Tests

Update existing tests and add new tests to cover new sensor logic

…pe enum

…_bgc

…ensors for instruments

…ated validations for each instrument

…igher level for scalability

… kernels from the argo vertical movement kernel to enable easier scalability

…operty shorthands

…pe enum

…_bgc

…ensors for instruments

…ated validations for each instrument

…igher level for scalability

…operty shorthands

…p into refactor-sensors

…figurations

for more information, see https://pre-commit.ci

…p into refactor-sensors

erikvansebille

Looks good! A few comments below

erikvansebille · 2026-04-13T06:18:24Z

src/virtualship/instruments/ctd_bgc.py

There seems to still be a lot of duplication between CTD and CTD_BGC. Was the intention for this PR not to remove that duplication/ Or will that come in a next PR?

Yes, indeed it will come in the next PR... the CTD_BGC will soon be removed but I decided not to do it in this PR so as to keep it focused

erikvansebille · 2026-04-13T06:20:38Z

src/virtualship/instruments/drifter.py

-        Variable("lifetime", dtype=np.float32),
-    ]
-)
+_DRIFTER_FIXED_VARIABLES = [


Not sure whether FIXED is the right term here (and in other instruments). Suggests they are fixed to the instrument. Would NONSENSOR be a better term? Or do you have another term?

erikvansebille · 2026-04-13T06:22:24Z

src/virtualship/instruments/sensors.py

+    TEMPERATURE = "TEMPERATURE"
+    SALINITY = "SALINITY"
+    VELOCITY = "VELOCITY"
+    OXYGEN = "OXYGEN"
+    CHLOROPHYLL = "CHLOROPHYLL"
+    NITRATE = "NITRATE"
+    PHOSPHATE = "PHOSPHATE"
+    PH = "PH"
+    PHYTOPLANKTON = "PHYTOPLANKTON"
+    PRIMARY_PRODUCTION = "PRIMARY_PRODUCTION"


Why is this list needed? Can we do without? Seems like an extra place to change when adding a sensor..

erikvansebille · 2026-04-13T06:23:51Z

src/virtualship/instruments/ship_underwater_st.py

+_ST_SENSOR_KERNELS: dict[SensorType, callable] = {
+    SensorType.TEMPERATURE: _sample_temperature,
+    SensorType.SALINITY: _sample_salinity,
+}


Why explicitly mention them here, if they are also part of sensors.py? Seems a duplication?

erikvansebille · 2026-04-13T06:25:04Z

src/virtualship/models/expedition.py

 import yaml

 from virtualship.errors import InstrumentsConfigError, ScheduleError
+from virtualship.instruments.sensors import (


Why not import all? So that if a new sensor is added, it can automatically also be imported here?

erikvansebille · 2026-04-13T06:28:45Z

src/virtualship/models/expedition.py

+        """
+        FieldSet-key → Copernicus-variable mapping for enabled sensors.
+
+        VELOCITY is a special case: one sensor provides two FieldSet variables (U and V).


But in Parcels, it should sample fieldset.UV, which is one Field?

Oops! Overcomplicated things here unnecessarily

erikvansebille · 2026-04-13T06:29:53Z

src/virtualship/models/expedition.py

+        ],
+        description=(
+            "Sensors fitted to the BGC CTD. "
+            "Supported: CHLOROPHYLL, NITRATE, OXYGEN, PH, PHOSPHATE, PHYTOPLANKTON, PRIMARY_PRODUCTION. "


Make this list the same order as the list above? Otherwise, more difficult to see whether the list is complete

erikvansebille · 2026-04-13T06:31:10Z

src/virtualship/models/expedition.py

+    @pydantic.field_validator("sensors", mode="after")
+    @classmethod
+    def _check_sensor_compatibility(cls, value) -> list[SensorConfig]:
+        return _check_sensor_compatibility(value, XBT_SUPPORTED_SENSORS, "XBT")
+
+    @pydantic.field_serializer("sensors")
+    def _serialize_sensors(self, value: list[SensorConfig], _info):
+        return _serialize_sensor_list(value)
+
+    def active_variables(self) -> dict[str, str]:
+        """FieldSet-key → Copernicus-variable mapping for enabled sensors."""
+        return build_variables_from_sensors(self.sensors)
+


Why are these functions required for every instrument? Can't they be reused?

erikvansebille · 2026-04-13T06:36:21Z

src/virtualship/utils.py

+            particle_vars=[
+                "U",
+                "V",
+            ],  # two particle variables associated with one sensor


Why not UV?

erikvansebille · 2026-04-13T06:37:31Z

tests/instruments/test_argo_float.py

Great to add so much more unit testing!

j-atkins added 30 commits March 25, 2026 13:53

remove CTD_BGC instrument type from InstrumentType enum, add SensorTy…

ba585c2

…pe enum

update utils: add sensor def mapping and remove old references to ctd…

1f7e9b8

…_bgc

refactor: update SensorType enum and add source-truth for supported s…

dbaa319

…ensors for instruments

add sensors configuration for various instruments

a2a7c81

new registries and helper functions

67a04d8

update expedition models, now including SensorConfig model and associ…

057d9f9

…ated validations for each instrument

modify adcp instrument class, also abstract expansion to u and v to h…

b82118d

…igher level for scalability

dynamic particle class building takes JIT or Scipy particle

8d96d8f

raise error when instrument has zero sensors enabled

818e8f8

use centralised particle class builder for ADCP now as well

07c8461

batch update instrument subclasses adapted to refactored sensor logic

1bf517e

rename list

961f1fe

adapt argo subclass to sensor refactoring, also separate the sampling…

ee002d7

… kernels from the argo vertical movement kernel to enable easier scalability

consistent particle variable naming

4777217

add back in ctd_bgc for now

c419399

fix import

daabfc4

move sensor information to new sensors.py file

cece7bb

update imports across codebase

b429331

add validator/serialiser for reading from YAML, remove unnecessary pr…

882a419

…operty shorthands

re-add JITParticle to particle class when creating instruments

126ecc2

remove CTD_BGC instrument type from InstrumentType enum, add SensorTy…

9011b2d

…pe enum

update utils: add sensor def mapping and remove old references to ctd…

464b3e9

…_bgc

refactor: update SensorType enum and add source-truth for supported s…

2f7d82d

…ensors for instruments

add sensors configuration for various instruments

1d7c158

new registries and helper functions

f01bf0e

update expedition models, now including SensorConfig model and associ…

d9f9d10

…ated validations for each instrument

modify adcp instrument class, also abstract expansion to u and v to h…

c50c43f

…igher level for scalability

dynamic particle class building takes JIT or Scipy particle

bb91f0c

raise error when instrument has zero sensors enabled

b584d70

use centralised particle class builder for ADCP now as well

6fb6284

j-atkins and others added 25 commits April 8, 2026 14:28

consistent particle variable naming

0962261

add back in ctd_bgc for now

c9d0623

fix import

8151a60

move sensor information to new sensors.py file

892c75d

update imports across codebase

13ded3f

add validator/serialiser for reading from YAML, remove unnecessary pr…

a30f72b

…operty shorthands

re-add JITParticle to particle class when creating instruments

f0f8a19

Merge branch 'refactor-sensors' of github.com:OceanParcels/virtualshi…

f245288

…p into refactor-sensors

update with new analysis environment

92fe4a1

Merge branch 'new-pixi-env' into refactor-sensors

f4c6205

Merge branch 'main' into refactor-sensors

4351657

fix erroneous sampling during descent and drift

089d2b6

update docstring

96ff22d

Add sensor configuration tests for various instruments and update con…

73474e9

…figurations

new tests for instruments, focused on new sensor logic

c5ec69b

new test_sensors suite

7e9c5e9

new tests for new sensor logic utils

18c1a2d

remove some overlap/duplication of tests

89e184a

deal with circular import issues

bf61220

add ctd_bgc stationkeeping logic back in (accidentally removed earlier)

51f4457

Merge branch 'main' into refactor-sensors

bcbc28f

[pre-commit.ci] auto fixes from pre-commit.com hooks

f9f17f2

for more information, see https://pre-commit.ci

correct comment

548297b

update docstrings, rethink some testing

7a163f7

Merge branch 'refactor-sensors' of github.com:OceanParcels/virtualshi…

6978c95

…p into refactor-sensors

j-atkins marked this pull request as ready for review April 10, 2026 13:34

j-atkins requested review from VeckoTheGecko, ammedd and erikvansebille April 10, 2026 13:34

erikvansebille reviewed Apr 13, 2026

View reviewed changes

Conversation

j-atkins commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Major changes

API change

Additional change

Follow-up PRs

Tests

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

j-atkins commented Apr 10, 2026 •

edited

Loading