Fixes #20: Handle missing id in inputs

pycoupler/__init__.py

@@ -32,7 +32,6 @@
     detect_io_type,
 )
 
-
 __all__ = [
     "LpjmlConfig",
     "CoupledConfig",

pycoupler/config.py

@@ -4,6 +4,7 @@
 import sys
 import subprocess
 import json
+import warnings
 from subprocess import run
 from ruamel.yaml import YAML
 
@@ -330,6 +331,10 @@ def set_coupled(
         self.sim_path = create_subdirs(sim_path, self.sim_name)
         output_path = f"{sim_path}/output/{self.sim_name}"
 
+        # The coupling depends on ids in the inputs.cjson,
+        # therefore, they need to be set, if they are missing
+        self._ensure_input_ids()
+
         # set time range for coupled run
         self._set_timerange(
             start_year=start_year, end_year=end_year, write_start_year=start_year
@@ -554,6 +559,46 @@ def _set_coupling(
         else:
             self.start_coupling = self.firstyear
 
+    def _ensure_input_ids(self) -> None:
+        """
+        Ensure that all inputs in the config have a unique id.
+        Warns on missing or duplicate ids, but sets new ones.
+        """
+        available_ids = set()
+        input_dict = self.input.to_dict()
+        # As a guess of the next free id, use the number of inputs + 1
+        next_free_id = len(input_dict) + 1
+
+        def find_new_id():
+            nonlocal next_free_id
+            new_id = next_free_id
+            while new_id in available_ids:
+                new_id += 1
+            next_free_id = new_id + 1
+            return new_id
+
+        for key, inp in input_dict.items():
+            if "id" not in inp:
+                # Missing id
+                warnings.warn(
+                    f"Input '{key}' is missing an id. Setting a new one for now."
+                )
+                id = find_new_id()
+                setattr(getattr(self.input, key), "id", id)
+            elif inp["id"] in available_ids:
+                # Duplicate id
+                id = find_new_id()
+                warnings.warn(
+                    f"Inputs contain duplicate ids. Violating input: '{key}' (id: '{inp["id"]}')"
+                )
+                setattr(getattr(self.input, key), "id", id)
+            else:
+                id = inp["id"]
+                if id >= next_free_id:
+                    next_free_id = id + 1
+
+            available_ids |= {id}
+
     def _set_input_sockets(self, inputs=[]):
         """Set sockets for inputs and outputs (via corresponding ids)"""
         for inp in inputs:
@@ -1135,12 +1180,10 @@ def __repr__(self, sub_repr=1, order=1):
 
         for key, value in self.__dict__.items():
             if isinstance(value, SubConfig):
-                summary += (
-                    f"""{'  ' * sub_repr}* {key}: {value.__repr__(
+                summary += f"""{'  ' * sub_repr}* {key}: {
+                    value.__repr__(
                         sub_repr + 1, order + 1
-                    )}""".strip()
-                    + spacing
-                )
+                    )}""".strip() + spacing
             else:
                 summary += (
                     f"{'  ' * sub_repr}* {key:<20} {value}".strip() + spacing

pycoupler/coupler.py

@@ -1043,7 +1043,7 @@ def _init_input(self):
             self._input_ids = {
                 input_sockets[inp]["id"]: inp
                 for inp in input_sockets
-                if input_sockets[inp]["id"] in LPJmLInputType.ids
+                if input_sockets[inp]["id"] in LPJmLInputType.ids_to_names
             }
 
             # send number of bands for each output data stream
@@ -1172,7 +1172,7 @@ def _get_config_input_sockets(self):
         sockets = self._config.get_input_sockets()
         socket_ids = [sock_id["id"] for sock_id in sockets.values()]
         # filter input names
-        input_ids = [inp for inp in LPJmLInputType.ids]
+        input_ids = [inp for inp in LPJmLInputType.ids_to_names]
 
         # check if input is defined in LPJmLInputType (band size required)
         valid_inputs = {
@@ -1184,7 +1184,7 @@ def _get_config_input_sockets(self):
             self.close()
             raise ValueError(
                 f"Configurated sockets {sockets.keys()} not defined in"
-                + f" {LPJmLInputType.names}!"
+                + f" {LPJmLInputType.ids_to_names.values()}!"
             )
         return valid_inputs
 

pycoupler/data.py

@@ -8,6 +8,7 @@
 from scipy.spatial import KDTree
 from xarray.core.variable import Variable
 import xarray.core.utils as utils
+import warnings
 
 from pycoupler.utils import read_json
 
@@ -32,6 +33,7 @@ class LPJmLInputType:
     """
 
     __input_types__ = None  # This will hold the configuration data
+    ids_to_names = {}
 
     def __init__(self, id=None, name=None):
         """Initialize the instance with an id (index)."""
@@ -41,32 +43,42 @@ def __init__(self, id=None, name=None):
             )
 
         if id is not None:
-            # Find the corresponding input id from the provided id
-            self.__dict__.update(
-                next(
-                    value.update({"name": key}) or value
-                    for key, value in LPJmLInputType.__input_types__.items()
-                    if value["id"] == id
-                )
-            )
+            name = LPJmLInputType.ids_to_names.get(id, None)
+            input_type = LPJmLInputType.__input_types__.get(name, None)
+            if input_type:
+                self.name = name
+                self.id = input_type["id"]
+                self.filename = input_type.get("name", None)
+                self.fmt = input_type.get("fmt", None)
+            else:
+                raise ValueError(f"Provided id '{id}' has no registered input type.")
         elif name is not None:
-            # Find the corresponding input name from the provided name
-            self.__dict__.update(
-                next(
-                    value.update({"name": key}) or value
-                    for key, value in LPJmLInputType.__input_types__.items()
-                    if key == name
+            input_type = LPJmLInputType.__input_types__.get(name, None)
+            if input_type:
+                self.name = name
+                self.id = input_type["id"]
+                self.filename = input_type.get("name", None)
+                self.fmt = input_type.get("fmt", None)
+            else:
+                raise ValueError(
+                    f"Provided name '{name}' is not a registered input type."
                 )
-            )
         else:
             raise ValueError("Either 'id' or 'name' must be provided.")
 
     @classmethod
     def load_config(cls, config):
         """Load input types from the provided config."""
-        cls.__input_types__ = config.input.to_dict()
-        cls.names = list(cls.__input_types__.keys())
-        cls.ids = [value["id"] for value in cls.__input_types__.values()]
+        cls.__input_types__ = {}
+        cls.ids_to_names = {}
+        for name, input_type in config.input.to_dict().items():
+            if "id" in input_type:
+                cls.ids_to_names[input_type["id"]] = name
+                cls.__input_types__[name] = input_type
+            else:
+                warnings.warn(
+                    f"Input type '{name}' is missing the required id field. It will be ignored."
+                )
 
     @property
     def nband(self):

tests/conftest.py

@@ -12,11 +12,11 @@ def test_path():
 
 
 @pytest.fixture
-def lpjml_coupler(config_coupled):
+def lpjml_coupler(config_coupled_json):
     os.environ["TEST_LINE_COUNTER"] = "0"
     # Using yield enables safe teardown of the fixture
     # (see https://docs.pytest.org/en/stable/how-to/fixtures.html#safe-teardowns)
-    yield LPJmLCoupler(config_file=config_coupled)
+    yield LPJmLCoupler(config_file=config_coupled_json)
     # Reset test line env variable
     os.environ["TEST_LINE_COUNTER"] = "0"
 
@@ -57,7 +57,7 @@ def outputpath_helper(output_dict, path):
 
 
 @pytest.fixture()
-def config_coupled(sim_path, model_path, test_path, sim_inputs, output_path):
+def config_coupled_json(sim_path, model_path, test_path, sim_inputs, output_path):
     new_config = sim_path / "config_coupled.json"
     with open(f"{test_path}/data/config_coupled_test.json") as conf:
         conf_d = json.load(conf)
@@ -68,7 +68,20 @@ def config_coupled(sim_path, model_path, test_path, sim_inputs, output_path):
         ]
         with new_config.open("w") as f:
             json.dump(conf_d, f)
-            return str(new_config)
+    return str(new_config)
+
+
+@pytest.fixture()
+def lpjml_config_json(sim_path, model_path, test_path, sim_inputs, output_path):
+    new_config = sim_path / "lpjml_config.json"
+    with open(f"{test_path}/data/lpjml_config.json") as conf:
+        conf_d = json.load(conf)
+        conf_d["output"] = [
+            outputpath_helper(out, str(output_path)) for out in conf_d["output"]
+        ]
+        with new_config.open("w") as f:
+            json.dump(conf_d, f)
+    return str(new_config)
 
 
 def pytest_configure(config):

tests/data/config_coupled_test.json

@@ -15387,7 +15387,7 @@
       "name": "input_VERSION2/lwnet_erainterim_1901-2011.clm"
     },
     "lwdown": {
-      "id": 43,
+      "id": 45,
       "fmt": "clm",
       "name": "DUMMYLOCATION"
     },
@@ -15407,7 +15407,7 @@
       "name": "input_VERSION2/mwindspeed_1860-2100_67420.clm"
     },
     "tamp": {
-      "id": 3,
+      "id": 46,
       "fmt": "clm",
       "name": "CRUDATA_TS3_23/cru_ts3.23.1901.2014.dtr.dat.clm"
     },

tests/data/lpjml_config.json

@@ -15387,7 +15387,7 @@
       "name": "input_VERSION2/lwnet_erainterim_1901-2011.clm"
     },
     "lwdown": {
-      "id": 43,
+      "id": 45,
       "fmt": "clm",
       "name": "DUMMYLOCATION"
     },
@@ -15407,7 +15407,7 @@
       "name": "input_VERSION2/mwindspeed_1860-2100_67420.clm"
     },
     "tamp": {
-      "id": 3,
+      "id": 46,
       "fmt": "clm",
       "name": "CRUDATA_TS3_23/cru_ts3.23.1901.2014.dtr.dat.clm"
     },

tests/test_config.py

@@ -1,6 +1,8 @@
 """Test the LPJmLConfig class."""
 
 from pycoupler.config import read_config, read_yaml, CoupledConfig, parse_config
+import json
+import pytest
 
 
 def test_set_spinup_config(test_path):
@@ -69,19 +71,19 @@ def test_set_historic_config(test_path):
     assert config_historic.double_harvest is False
 
 
-def test_set_coupled_config(test_path):
+def test_set_coupled_config(
+    lpjml_config_json, config_coupled_json, model_path, sim_path, output_path
+):
     """Test the set_config method of the LPJmLCoupler class."""
     # create config for coupled run
-    config_coupled = read_config(
-        model_path=f"{test_path}/data", file_name="lpjml_config.json"
-    )
+    config_coupled = read_config(model_path=model_path, file_name=lpjml_config_json)
 
     config_coupled.startgrid = 27410
     config_coupled.endgrid = 27411
 
     # set coupled run configuration
     config_coupled.set_coupled(
-        sim_path=f"{test_path}/data",
+        sim_path=sim_path,
         sim_name="coupled_test",
         dependency="historic_run",
         start_year=2001,
@@ -128,10 +130,10 @@ def test_set_coupled_config(test_path):
 
     # create config for coupled run
     check_config_coupled = read_config(
-        model_path=f"{test_path}/data", file_name="config_coupled_test.json"
+        model_path=model_path, file_name=config_coupled_json
     )
     # update with actual output path (test directory)
-    check_config_coupled._set_outputpath(f"{test_path}/data/output/coupled_test")
+    check_config_coupled._set_outputpath(output_path)
 
     # align both config objects
     check_config_coupled.restart_filename = config_coupled.restart_filename
@@ -145,11 +147,12 @@ def test_set_coupled_config(test_path):
 
     assert (
         repr(config_coupled)
-        == f"<pycoupler.LpjmlConfig>\nSettings:      lpjml v5.8\n  (general)\n  * sim_name   coupled_test\n  * firstyear  2001\n  * lastyear   2050\n  * startgrid  27410\n  * endgrid    27411\n  * landuse    yes\n  (changed)\n  * model_path           {test_path}/data\n  * sim_path             {test_path}/data\n  * outputyear           2022\n  * output_metafile      True\n  * grid_type            float\n  * write_restart        False\n  * nspinup              0\n  * float_grid           True\n  * restart_filename     {test_path}/data/restart/restart_historic_run.lpj\n  * outputyear           2022\n  * radiation            cloudiness\n  * fix_co2              True\n  * fix_co2_year         2018\n  * fix_climate          True\n  * fix_climate_cycle    11\n  * fix_climate_year     2013\n  * river_routing        False\n  * tillage_type         read\n  * residue_treatment    fixed_residue_remove\n  * double_harvest       False\n  * intercrop            True\nCoupled model:        copan:CORE\n  * start_coupling    2023\n  * input (coupled)   ['with_tillage']\n  * output (coupled)  ['grid', 'pft_harvestc', 'cftfrac', 'soilc_agr_layer', 'hdate', 'country', 'region']\n"  # noqa
+        == f"<pycoupler.LpjmlConfig>\nSettings:      lpjml v5.8\n  (general)\n  * sim_name   coupled_test\n  * firstyear  2001\n  * lastyear   2050\n  * startgrid  27410\n  * endgrid    27411\n  * landuse    yes\n  (changed)\n  * model_path           {str(model_path)}\n  * sim_path             {str(sim_path)}\n  * outputyear           2022\n  * output_metafile      True\n  * grid_type            float\n  * write_restart        False\n  * nspinup              0\n  * float_grid           True\n  * restart_filename     {str(sim_path)}/restart/restart_historic_run.lpj\n  * outputyear           2022\n  * radiation            cloudiness\n  * fix_co2              True\n  * fix_co2_year         2018\n  * fix_climate          True\n  * fix_climate_cycle    11\n  * fix_climate_year     2013\n  * river_routing        False\n  * tillage_type         read\n  * residue_treatment    fixed_residue_remove\n  * double_harvest       False\n  * intercrop            True\nCoupled model:        copan:CORE\n  * start_coupling    2023\n  * input (coupled)   ['with_tillage']\n  * output (coupled)  ['grid', 'pft_harvestc', 'cftfrac', 'soilc_agr_layer', 'hdate', 'country', 'region']\n"  # noqa
     )  # noqa
+    assert config_coupled_dict["input"] == check_config_coupled_dict["input"]
     assert config_coupled_dict == check_config_coupled_dict
 
-    config_coupled.sim_path = f"{test_path}/data"
+    config_coupled.sim_path = sim_path
     assert config_coupled.convert_cdf_to_raw() == "tested"
 
     assert {
@@ -191,12 +194,47 @@ def test_read_config(test_path):
     assert coupled_config.__class__.__name__ == "LpjmlConfig"
 
 
-def test_parse_config(test_path):
-    coupled_config = parse_config(f"{test_path}/data/lpjml_config.json")
+def test_parse_config(lpjml_config_json):
+    coupled_config = parse_config(lpjml_config_json)
     assert coupled_config["model_path"] == "LPJmL_internal"
     assert coupled_config["coupled_model"] is None
 
-    coupled_config = parse_config(
-        f"{test_path}/data/lpjml_config.json", config_class=CoupledConfig
-    )
+    coupled_config = parse_config(lpjml_config_json, config_class=CoupledConfig)
     assert coupled_config.__class__.__name__ == "CoupledConfig"
+
+
+@pytest.fixture(
+    params=[
+        {"fmt": "clm", "name": "test/test.clm"},
+        {"id": 1, "fmt": "clm", "name": "test/test.clm"},
+        pytest.param(
+            {"id": 2, "fmt": "clm", "name": "test/test.clm"}, marks=pytest.mark.xfail
+        ),
+    ],
+    ids=["no_id", "duplicate_id", "no_errors"],
+)
+def lpjml_config_wrong_ids(request, lpjml_config_json):
+    with open(lpjml_config_json, "r+") as conf:
+        conf_d = json.load(fp=conf)
+        conf_d["input"] = {
+            "test": request.param,
+            "test2": {"id": 1, "fmt": "clm", "name": "input/test2.clm"},
+        }
+        conf.seek(0)
+        json.dump(conf_d, conf)
+        conf.truncate()
+    return str(lpjml_config_json)
+
+
+def test_wrong_ids(lpjml_config_wrong_ids, sim_path):
+    config_coupled = read_config(lpjml_config_wrong_ids)
+
+    with pytest.warns(UserWarning):
+        config_coupled._ensure_input_ids()
+
+    inputs = config_coupled.input.to_dict()
+    ids = []
+    for inp in inputs.values():
+        assert "id" in inp, "Not every entry has an ID"
+        ids.append(inp["id"])
+    assert len(ids) == len(set(ids)), "IDs are not unique"