Clearer error message when the column Instrument is missing (#134)

* Clearer error message when the column Instrument is missing

* Fixing mfp unit test

* Also adding error when instrument name is not valid

* Quickfix for issue where instruments from mfp is a list

* Fixing for unit tests

* Adding ship-download when ADCP or underwater in config

Author: erikvansebille

src/virtualship/cli/commands.py

@@ -15,7 +15,11 @@
     get_space_time_region_hash,
     hash_to_filename,
 )
-from virtualship.expedition.do_expedition import _get_schedule, do_expedition
+from virtualship.expedition.do_expedition import (
+    _get_schedule,
+    _get_ship_config,
+    do_expedition,
+)
 from virtualship.utils import SCHEDULE, SHIP_CONFIG, mfp_to_yaml
 
 
@@ -107,6 +111,7 @@ def fetch(path: str | Path, username: str | None, password: str | None) -> None:
     data_folder.mkdir(exist_ok=True)
 
     schedule = _get_schedule(path)
+    ship_config = _get_ship_config(path)
 
     if schedule.space_time_region is None:
         raise ValueError(
@@ -131,7 +136,10 @@ def fetch(path: str | Path, username: str | None, password: str | None) -> None:
     start_datetime = time_range.start_time
     end_datetime = time_range.end_time
     instruments_in_schedule = [
-        waypoint.instrument.name for waypoint in schedule.waypoints
+        waypoint.instrument[0].name
+        if isinstance(waypoint.instrument, list)
+        else waypoint.instrument.name
+        for waypoint in schedule.waypoints  # TODO check why instrument is a list here
     ]
 
     # Create download folder and set download metadata
@@ -142,7 +150,11 @@ def fetch(path: str | Path, username: str | None, password: str | None) -> None:
     )
     shutil.copyfile(path / SCHEDULE, download_folder / SCHEDULE)
 
-    if set(["XBT", "CTD", "SHIP_UNDERWATER_ST"]) & set(instruments_in_schedule):
+    if (
+        (set(["XBT", "CTD", "SHIP_UNDERWATER_ST"]) & set(instruments_in_schedule))
+        or hasattr(ship_config, "ship_underwater_st_config")
+        or hasattr(ship_config, "adcp_config")
+    ):
         print("Ship data will be downloaded")
 
         # Define all ship datasets to download, including bathymetry

src/virtualship/expedition/do_expedition.py

@@ -42,8 +42,6 @@ def do_expedition(expedition_dir: str | Path, input_data: Path | None = None) ->
         "DRIFTER",
         "XBT",
         "CTD",
-        "ADCP",
-        "SHIP_UNDERWATER_ST",
     ]:  # TODO make instrument names consistent capitals or lowercase throughout codebase
         if (
             hasattr(ship_config, instrument.lower() + "_config")
@@ -182,6 +180,15 @@ def _get_schedule(expedition_dir: Path) -> Schedule:
         raise FileNotFoundError(f'Schedule not found. Save it to "{file_path}".') from e
 
 
+def _get_ship_config(expedition_dir: Path) -> Schedule:
+    """Load Schedule object from yaml config file in `expedition_dir`."""
+    file_path = expedition_dir.joinpath(SHIP_CONFIG)
+    try:
+        return ShipConfig.from_yaml(file_path)
+    except FileNotFoundError as e:
+        raise FileNotFoundError(f'Config not found. Save it to "{file_path}".') from e
+
+
 def _load_checkpoint(expedition_dir: Path) -> Checkpoint | None:
     file_path = expedition_dir.joinpath(CHECKPOINT)
     try:

src/virtualship/utils.py

@@ -75,6 +75,11 @@ def mfp_to_yaml(excel_file_path: str, yaml_output_path: str):  # noqa: D417
     # Check if the headers match the expected ones
     actual_columns = set(coordinates_data.columns)
 
+    if "Instrument" not in actual_columns:
+        raise ValueError(
+            "Error: Missing column 'Instrument'. Have you added this column after exporting from MFP?"
+        )
+
     missing_columns = expected_columns - actual_columns
     if missing_columns:
         raise ValueError(
@@ -134,9 +139,18 @@ def mfp_to_yaml(excel_file_path: str, yaml_output_path: str):  # noqa: D417
     # Generate waypoints
     waypoints = []
     for _, row in coordinates_data.iterrows():
-        instruments = [
-            InstrumentType(instrument) for instrument in row["Instrument"].split(", ")
-        ]
+        try:
+            instruments = [
+                InstrumentType(instrument)
+                for instrument in row["Instrument"].split(", ")
+            ]
+        except ValueError as err:
+            raise ValueError(
+                f"Error: Invalid instrument type in row {row.name}. "
+                "Please ensure that the instrument type is one of: "
+                f"{[instrument.name for instrument in InstrumentType]}. "
+                "Also be aware that these are case-sensitive."
+            ) from err
         waypoints.append(
             Waypoint(
                 instrument=instruments,

tests/test_mfp_to_yaml.py

@@ -58,7 +58,8 @@ def test_mfp_to_yaml_missing_headers(mock_read_excel, tmp_path):
     yaml_output_path = tmp_path / "schedule.yaml"
 
     with pytest.raises(
-        ValueError, match="Error: Found columns .* but expected columns .*"
+        ValueError,
+        match="Error: Missing column 'Instrument'. Have you added this column after exporting from MFP?",
     ):
         mfp_to_yaml("mock_file.xlsx", yaml_output_path)