Refactor: instruments have sensors

[j-atkins]

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

[erikvansebille]

Looks good! A few comments below

[erikvansebille]

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

[j-atkins]

Oops! Overcomplicated things here unnecessarily

[j-atkins]

Update: I think there is still a need to separate it out here because it relates to drawing these two separate fields from the Copernicus Marine Service

[VeckoTheGecko]

Good work so far! Really like the direction that this is going. I have a bunch of comments, some quick fixes and others more cenceptual that I think would be good to go over together.

[VeckoTheGecko]

I think it would be good to rename this class to _Sensor, and to include the type in the class itself

Suggested change

	class _SensorMeta:
	fs_key: str # map to Parcels fieldset variables
	copernicus_var: str # map to Copernicus Marine Service variable names
	class _Sensor:
	type_: SensorType
	fs_key: str # map to Parcels fieldset variables
	copernicus_var: str # map to Copernicus Marine Service variable names

Then the SENSOR_REGISTRY can become:

SENSOR_REGISTRY: dict[SensorType, _Sensor] = {s.type_: s for s in [
    ... # list of sensors
]} 

making it easy to look up sensors, but also making it so that if a sensor its chosen it's easy to see what type it is again.

[VeckoTheGecko]

I find it a bit confusing the structure of the classes here - perhaps this is something worth drawing up so that we have some clarity?

We have an Instrument class

@register_instrument(InstrumentType.ADCP)
class ADCPInstrument(Instrument):
    """ADCP instrument class."""

    def __init__(self, expedition, from_data):
        """Initialize ADCPInstrument."""
        variables = expedition.instruments_config.adcp_config.active_variables()
        limit_spec = {
            "spatial": True
        }  # spatial limits; lat/lon constrained to waypoint locations + buffer

        super().__init__(
            expedition,
            variables,
            add_bathymetry=False,
            allow_time_extrapolation=True,
            verbose_progress=False,
            spacetime_buffer_size=None,
            limit_spec=limit_spec,
            from_data=from_data,
        )

But I feel that there's a lot of details in this class that aren't really related to what an instrument is, but is more glue code to get it working in VirtualShip

From my POV, its best to design things close to the physical domain. An Instrument instance has a set of sensors installed on the machine. These sensors need to match up with a list of allowed sensors (which are defined on the class, e.g., via a method .get_allowed_sensors() and a check in the __init__ that it adheres - this would remove all the _check_sensor_compatibility methods that are currently here). Then each sensor has its own details potentially related to that sensor.

Maybe it would be good for us to sit down and diagram out the abstractions here. Part of that might be brainstorming how the interface between the configs and model code looks like.

[VeckoTheGecko]

Is it possible for us to also add the kernels to this Sensor class? I see a bunch of mappings across the codebase relating the sensors to the kernels (where a lot of them are the same, as Erik mentioned in #328 (comment) ).

I think it would be a big win if we can put the kernels in the sensor class! I don't know how easy it would be since the right kernel might depend on more than just the choice of sensor. If that's the case - what does choosing the right kernel depend on?

[VeckoTheGecko]

Should we move this to sensors.py? The class and the SENSOR_REGISTRY?

[VeckoTheGecko]

I see that this is only used in

virtualship/src/virtualship/utils.py

Lines 746 to 751 in 89e184a

	sensor_variables = [
	Variable(var_name, dtype=np.float32, initial=np.nan)
	for sc in sensors
	if sc.enabled
	for var_name in sc.meta.particle_vars
	]

. Perhaps it would be clearer instead of these being strings for them to explicitly be lists of parcels.Variable objects?

Also, for my own understanding, how does this relate to (e.g.)

virtualship/src/virtualship/instruments/argo_float.py

Lines 38 to 49 in 89e184a

	_ARGO_FIXED_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("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),
	Variable("grounded", dtype=np.int32, initial=0),
	]

? Are the particle variables here "extra ones" needed for the sensor and to be captured as output, whereas the ones in _ARGO_FIXED_VARIABLES are for the base behaviour of the instrument?

[j-atkins]

Are the particle variables here "extra ones" needed for the sensor and to be captured as output, whereas the ones in _ARGO_FIXED_VARIABLES are for the base behaviour of the instrument?

Yes indeed that's it, and now as Erik suggests this will be renamed to *_NONSENSOR_VARIABLES.

[VeckoTheGecko]

This type hint isn't correct

Suggested change

	_ADCP_SENSOR_KERNELS: dict[SensorType, callable] = {
	SensorType.VELOCITY: _sample_velocity,
	}
	from collections.abc.Callable # put at top of file
	_ADCP_SENSOR_KERNELS: dict[SensorType, Callable] = {
	SensorType.VELOCITY: _sample_velocity,
	}

---

Great use of typing across the PR - I think having this typing is really useful so that it gets us to think more about the structure of the codebase. Currently I don't think typechecking is run in CI (or if it is, we currently have a lot of failures). Should we map out a path forward for enabling typechecking across the codebase as well? The types are only really useful if they're enforced (something I'm working on in Parcels as well).

[j-atkins]

Yes sounds good... could similar implementation as in Parcels be carried over to VirtualShip?

[VeckoTheGecko]

Yes, same config etc can be used

[VeckoTheGecko]

Same here about the callable type annotation - perhaps worth find-all'ing across the codebase for , callable] so all these are replaced

[VeckoTheGecko]

Suggested change

	fixed_variables: list,
	fixed_variables: list[Variable],

[VeckoTheGecko]

nitpick. Feel free to ignore this suggestion if you don't find it useful

Suggested change

	particle_class: type,
	particle_class: type, # generic type annotation needed for v3 particle class behaviour # TODO: Update with Parcels v4

[j-atkins]

I have addressed various comments from the first round of review on this PR. See below for a list of main changes:

• Renamed all _*_FIXED_VARIABLES lists to _*_NONSENSOR_VARIABLES
• Renamed _SensorMeta to _Sensor, added type_: SensorType as a field so each sensor object is self-describing, and moved the class + SENSOR_REGISTRY from utils.py into sensors.py
• New _InstrumentConfigMixin in expedition.py defines shared methods that can be flexibly inherited across the InstrumentConfig models, addresses duplication flagged.
  • Note, though, also that this logic has been extended to other preexisting datetime related parts of the InstrumetConfigs, e.g. minutes and lifetime params (so this is some 'bonus' refactoring).
  • The tests associated with each method are maintained and a new test added to test_expedition.py to check that all InstrumentConfig models inherit from the mixin.
• Instrument subclasses (e.g. CTDInstrument) must now define sensor_kernel dictionary to map the sensor types to the relevant sampling kernels as a class attribute. This is checked by the Instrument base class and also a test added to test_base.py. Means the allowlists for different sensor types are now explicitly defined and managed within each instrument class, which is nice for further centralising instrument characteristics and behaviours.