fix(commands): fix remove_cluster command and add unit tests for BC and area removal

antarest/study/storage/variantstudy/model/command/remove_area.py

@@ -93,13 +93,15 @@ def _remove_area_from_binding_constraints(self, study_data: FileStudy) -> None:
 
             Instead, we decide to remove the binding constraints that are related to the area.
         """
+        # See also `RemoveArea`
         # noinspection SpellCheckingInspection
         url = ["input", "bindingconstraints", "bindingconstraints"]
         binding_constraints = study_data.tree.get(url)
 
         # Collect the binding constraints that are related to the area to remove
         # by searching the terms that contain the ID of the area.
         bc_to_remove = {}
+        lower_area_id = self.id.lower()
         for bc_index, bc in list(binding_constraints.items()):
             for key in bc:
                 # Term IDs are in the form `area1%area2` or `area.cluster`
@@ -110,7 +112,8 @@ def _remove_area_from_binding_constraints(self, study_data: FileStudy) -> None:
                 else:
                     # This key belongs to the set of properties, it isn't a term ID, so we skip it
                     continue
-                if self.id.lower() in related_areas:
+                related_areas = [area.lower() for area in related_areas]
+                if lower_area_id in related_areas:
                     bc_to_remove[bc_index] = binding_constraints.pop(bc_index)
                     break
 

antarest/study/storage/variantstudy/model/command/remove_cluster.py

@@ -134,26 +134,50 @@ def _create_diff(self, other: "ICommand") -> t.List["ICommand"]:
     def get_inner_matrices(self) -> t.List[str]:
         return []
 
-    # noinspection SpellCheckingInspection
     def _remove_cluster_from_binding_constraints(self, study_data: FileStudy) -> None:
-        config = study_data.tree.get(["input", "bindingconstraints", "bindingconstraints"])
-
-        # Binding constraints IDs to remove
-        ids_to_remove = set()
-
-        # Cluster IDs are stored in lower case in the binding contraints configuration file.
-        cluster_id = self.cluster_id.lower()
-        for bc_id, bc_props in config.items():
-            if f"{self.area_id}.{cluster_id}" in bc_props.keys():
-                ids_to_remove.add(bc_id)
-
-        for bc_id in ids_to_remove:
-            study_data.tree.delete(["input", "bindingconstraints", config[bc_id]["id"]])
-            bc = next(iter([bind for bind in study_data.config.bindings if bind.id == config[bc_id]["id"]]))
-            study_data.config.bindings.remove(bc)
-            del config[bc_id]
-
-        study_data.tree.save(
-            config,
-            ["input", "bindingconstraints", "bindingconstraints"],
-        )
+        """
+        Remove the binding constraints that are related to the thermal cluster.
+
+        Notes:
+            A binding constraint has properties, a list of terms (which form a linear equation) and
+            a right-hand side (which is the matrix of the binding constraint).
+            The terms are of the form `area1%area2` or `area.cluster` where `area` is the ID of the area
+            and `cluster` is the ID of the cluster.
+
+            When a thermal cluster is removed, it has an impact on the terms of the binding constraints.
+            At first, we could decide to remove the terms that are related to the area.
+            However, this would lead to a linear equation that is not valid anymore.
+
+            Instead, we decide to remove the binding constraints that are related to the cluster.
+        """
+        # See also `RemoveCluster`
+        # noinspection SpellCheckingInspection
+        url = ["input", "bindingconstraints", "bindingconstraints"]
+        binding_constraints = study_data.tree.get(url)
+
+        # Collect the binding constraints that are related to the area to remove
+        # by searching the terms that contain the ID of the area.
+        bc_to_remove = {}
+        lower_area_id = self.area_id.lower()
+        lower_cluster_id = self.cluster_id.lower()
+        for bc_index, bc in list(binding_constraints.items()):
+            for key in bc:
+                if "." not in key:
+                    # This key identifies a link or belongs to the set of properties.
+                    # It isn't a cluster ID, so we skip it.
+                    continue
+                # Term IDs are in the form `area1%area2` or `area.cluster`
+                # noinspection PyTypeChecker
+                related_area_id, related_cluster_id = map(str.lower, key.split("."))
+                if (lower_area_id, lower_cluster_id) == (related_area_id, related_cluster_id):
+                    bc_to_remove[bc_index] = binding_constraints.pop(bc_index)
+                    break
+
+        matrix_suffixes = ["_lt", "_gt", "_eq"] if study_data.config.version >= 870 else [""]
+
+        for bc_index, bc in bc_to_remove.items():
+            for suffix in matrix_suffixes:
+                # noinspection SpellCheckingInspection
+                study_data.tree.delete(["input", "bindingconstraints", f"{bc['id']}{suffix}"])
+
+        study_data.tree.save(binding_constraints, url)

tests/variantstudy/conftest.py

@@ -8,6 +8,12 @@
 import numpy.typing as npt
 import pytest
 
+from antarest.study.storage.study_upgrader import get_current_version
+
+if t.TYPE_CHECKING:
+    # noinspection PyPackageRequirements
+    from _pytest.fixtures import SubRequest
+
 from antarest.matrixstore.model import MatrixDTO
 from antarest.matrixstore.service import MatrixService
 from antarest.matrixstore.uri_resolver_service import UriResolverService
@@ -134,27 +140,32 @@ def command_factory_fixture(matrix_service: MatrixService) -> CommandFactory:
 
 
 @pytest.fixture(name="empty_study")
-def empty_study_fixture(tmp_path: Path, matrix_service: MatrixService) -> FileStudy:
+def empty_study_fixture(request: "SubRequest", tmp_path: Path, matrix_service: MatrixService) -> FileStudy:
     """
     Fixture for creating an empty FileStudy object.
 
     Args:
+        request: pytest's request object.
         tmp_path: The temporary path for extracting the empty study.
         matrix_service: The MatrixService object.
 
     Returns:
         FileStudy: The empty FileStudy object.
     """
-    empty_study_path: Path = ASSETS_DIR / "empty_study_720.zip"
+    zip_name = getattr(request, "param", "empty_study_720.zip")
+    empty_study_path: Path = ASSETS_DIR / zip_name
     empty_study_destination_path = tmp_path.joinpath("empty-study")
     with zipfile.ZipFile(empty_study_path, "r") as zip_empty_study:
         zip_empty_study.extractall(empty_study_destination_path)
 
+    # Detect the version of the study from `study.antares` file.
+    version = get_current_version(empty_study_destination_path)
+
     config = FileStudyTreeConfig(
         study_path=empty_study_destination_path,
         path=empty_study_destination_path,
         study_id="",
-        version=720,
+        version=int(version),
         areas={},
         sets={},
     )

tests/variantstudy/model/command/test_manage_binding_constraints.py

@@ -1,12 +1,16 @@
 from unittest.mock import Mock
 
 import numpy as np
+import pytest
 
 from antarest.study.storage.rawstudy.ini_reader import IniReader
 from antarest.study.storage.rawstudy.model.filesystem.config.binding_constraint import BindingConstraintFrequency
 from antarest.study.storage.rawstudy.model.filesystem.factory import FileStudy
 from antarest.study.storage.variantstudy.business.command_extractor import CommandExtractor
 from antarest.study.storage.variantstudy.business.command_reverter import CommandReverter
+from antarest.study.storage.variantstudy.business.matrix_constants.binding_constraint.series_after_v87 import (
+    default_bc_weekly_daily as default_bc_weekly_daily_870,
+)
 from antarest.study.storage.variantstudy.business.matrix_constants.binding_constraint.series_before_v87 import (
     default_bc_hourly,
     default_bc_weekly_daily,
@@ -24,41 +28,18 @@
 
 
 # noinspection SpellCheckingInspection
-def test_manage_binding_constraint(
-    empty_study: FileStudy,
-    command_context: CommandContext,
-):
+@pytest.mark.parametrize("empty_study", ["empty_study_720.zip", "empty_study_870.zip"], indirect=True)
+def test_manage_binding_constraint(empty_study: FileStudy, command_context: CommandContext):
     study_path = empty_study.config.study_path
 
     area1 = "area1"
     area2 = "area2"
     cluster = "cluster"
-    CreateArea.parse_obj(
-        {
-            "area_name": area1,
-            "command_context": command_context,
-        }
-    ).apply(empty_study)
-    CreateArea.parse_obj(
-        {
-            "area_name": area2,
-            "command_context": command_context,
-        }
-    ).apply(empty_study)
-    CreateLink.parse_obj(
-        {
-            "area1": area1,
-            "area2": area2,
-            "command_context": command_context,
-        }
-    ).apply(empty_study)
+    CreateArea.parse_obj({"area_name": area1, "command_context": command_context}).apply(empty_study)
+    CreateArea.parse_obj({"area_name": area2, "command_context": command_context}).apply(empty_study)
+    CreateLink.parse_obj({"area1": area1, "area2": area2, "command_context": command_context}).apply(empty_study)
     CreateCluster.parse_obj(
-        {
-            "area_id": area1,
-            "cluster_name": cluster,
-            "parameters": {},
-            "command_context": command_context,
-        }
+        {"area_id": area1, "cluster_name": cluster, "parameters": {}, "command_context": command_context}
     ).apply(empty_study)
 
     bind1_cmd = CreateBindingConstraint(
@@ -83,10 +64,18 @@ def test_manage_binding_constraint(
     res2 = bind2_cmd.apply(empty_study)
     assert res2.status
 
-    bc1_matrix_path = study_path / "input/bindingconstraints/bd 1.txt.link"
-    bc2_matrix_path = study_path / "input/bindingconstraints/bd 2.txt.link"
-    assert bc1_matrix_path.exists()
-    assert bc2_matrix_path.exists()
+    if empty_study.config.version < 870:
+        matrix_links = ["bd 1.txt.link", "bd 2.txt.link"]
+    else:
+        matrix_links = [
+            # fmt: off
+            "bd 1_lt.txt.link", "bd 1_eq.txt.link", "bd 1_gt.txt.link",
+            "bd 2_lt.txt.link", "bd 2_eq.txt.link", "bd 2_gt.txt.link",
+            # fmt: on
+        ]
+    for matrix_link in matrix_links:
+        link_path = study_path / f"input/bindingconstraints/{matrix_link}"
+        assert link_path.exists(), f"Missing matrix link: {matrix_link!r}"
 
     cfg_path = study_path / "input/bindingconstraints/bindingconstraints.ini"
     bd_config = IniReader().read(cfg_path)
@@ -108,14 +97,26 @@ def test_manage_binding_constraint(
         "type": "daily",
     }
 
-    weekly_values = default_bc_weekly_daily.tolist()
+    if empty_study.config.version < 870:
+        weekly_values = default_bc_weekly_daily.tolist()
+        values = weekly_values
+        less_term_matrix = None
+        greater_term_matrix = None
+    else:
+        weekly_values = default_bc_weekly_daily_870.tolist()
+        values = None
+        less_term_matrix = weekly_values
+        greater_term_matrix = weekly_values
+
     bind_update = UpdateBindingConstraint(
         id="bd 1",
         enabled=False,
         time_step=BindingConstraintFrequency.WEEKLY,
         operator=BindingConstraintOperator.BOTH,
         coeffs={"area1%area2": [800, 30]},
-        values=weekly_values,
+        values=values,
+        less_term_matrix=less_term_matrix,
+        greater_term_matrix=greater_term_matrix,
         command_context=command_context,
     )
     res = bind_update.apply(empty_study)
@@ -133,7 +134,16 @@ def test_manage_binding_constraint(
     remove_bind = RemoveBindingConstraint(id="bd 1", command_context=command_context)
     res3 = remove_bind.apply(empty_study)
     assert res3.status
-    assert not bc1_matrix_path.exists()
+
+    for matrix_link in matrix_links:
+        link_path = study_path / f"input/bindingconstraints/{matrix_link}"
+        if matrix_link.startswith("bd 1"):
+            assert not link_path.exists(), f"Matrix link not removed: {matrix_link!r}"
+        elif matrix_link.startswith("bd 2"):
+            assert link_path.exists(), f"Matrix link removed: {matrix_link!r}"
+        else:
+            raise NotImplementedError(f"Unexpected matrix link: {matrix_link!r}")
+
     bd_config = IniReader().read(cfg_path)
     assert len(bd_config) == 1
     assert bd_config.get("0") == {

tests/variantstudy/model/command/test_remove_area.py

@@ -3,7 +3,6 @@
 from antarest.study.storage.rawstudy.model.filesystem.config.binding_constraint import BindingConstraintFrequency
 from antarest.study.storage.rawstudy.model.filesystem.config.model import transform_name_to_id
 from antarest.study.storage.rawstudy.model.filesystem.factory import FileStudy
-from antarest.study.storage.study_upgrader import upgrade_study
 from antarest.study.storage.variantstudy.model.command.common import BindingConstraintOperator
 from antarest.study.storage.variantstudy.model.command.create_area import CreateArea
 from antarest.study.storage.variantstudy.model.command.create_binding_constraint import CreateBindingConstraint
@@ -18,13 +17,8 @@
 
 
 class TestRemoveArea:
-    @pytest.mark.parametrize("version", [810, 840])
-    def test_apply(
-        self,
-        empty_study: FileStudy,
-        command_context: CommandContext,
-        version: int,
-    ):
+    @pytest.mark.parametrize("empty_study", ["empty_study_810.zip", "empty_study_840.zip"], indirect=True)
+    def test_apply(self, empty_study: FileStudy, command_context: CommandContext):
         # noinspection SpellCheckingInspection
         empty_study.tree.save(
             {
@@ -72,10 +66,8 @@ def test_apply(
 
         ########################################################################################
 
-        upgrade_study(empty_study.config.study_path, str(version))
-
         empty_study_cfg = empty_study.tree.get(depth=999)
-        if version >= 830:
+        if empty_study.config.version >= 830:
             empty_study_cfg["input"]["areas"][area_id]["adequacy_patch"] = {
                 "adequacy-patch": {"adequacy-patch-mode": "outside"}
             }
@@ -84,7 +76,6 @@ def test_apply(
         area_name2 = "Area2"
         area_id2 = transform_name_to_id(area_name2)
 
-        empty_study.config.version = version
         create_area_command: ICommand = CreateArea.parse_obj(
             {
                 "area_name": area_name2,

tests/variantstudy/model/command/test_remove_cluster.py

@@ -1,3 +1,5 @@
+import numpy as np
+import pytest
 from checksumdir import dirhash
 
 from antarest.study.storage.rawstudy.model.filesystem.config.binding_constraint import BindingConstraintFrequency
@@ -13,6 +15,7 @@
 
 
 class TestRemoveCluster:
+    @pytest.mark.parametrize("empty_study", ["empty_study_720.zip", "empty_study_870.zip"], indirect=True)
     def test_apply(self, empty_study: FileStudy, command_context: CommandContext):
         area_name = "Area_name"
         area_id = transform_name_to_id(area_name)
@@ -39,6 +42,15 @@ def test_apply(self, empty_study: FileStudy, command_context: CommandContext):
             modulation=[[0]],
         ).apply(empty_study)
 
+        # Binding constraint 2nd member: array of shape (8784, 3)
+        array = np.random.rand(8784, 3) * 1000
+        if empty_study.config.version < 870:
+            values = array.tolist()
+            less_term_matrix = None
+        else:
+            values = None
+            less_term_matrix = array.tolist()
+
         bind1_cmd = CreateBindingConstraint(
             name="BD 1",
             time_step=BindingConstraintFrequency.HOURLY,
@@ -48,6 +60,8 @@ def test_apply(self, empty_study: FileStudy, command_context: CommandContext):
             },
             comments="Hello",
             command_context=command_context,
+            values=values,
+            less_term_matrix=less_term_matrix,
         )
         output = bind1_cmd.apply(study_data=empty_study)
         assert output.status