Feature/ant 958 by ianmnz · Pull Request #12 · AntaresSimulatorTeam/GemsPy

ianmnz · 2024-02-15T17:18:41Z

A first working dev for the integration of Benders to the PoC

src/andromede/model/parameter.py

tbittar · 2024-02-19T08:27:54Z

src/andromede/simulation/optimization.py

+class SolverVariableInfo:
+    """
+    Helper class for constructing the structure file
+    for Bender solver. It keeps track of the corresponding


Benders (name of a mathematician)

tbittar · 2024-02-19T08:30:35Z

src/andromede/simulation/optimization.py

+    """
+
+    name: str
+    column: int


Maybe column_id or col_id is more explicit ?

tbittar · 2024-02-19T08:33:08Z

src/andromede/simulation/optimization.py

        key = TimestepComponentVariableKey(
            component_id, variable_name, block_timestep, scenario
        )
+        if key not in self._component_variables:


Could you use get_or_add defined in utils.py

Not quite. It checks a first dictionary and, doing so, it checks implicitly the second adding a new key to the second

tbittar · 2024-02-19T08:36:08Z

src/andromede/simulation/optimization.py

-                expression=instantiated_expression,
-            )
+        simulator = 0
+        xpansion_merged = 1


What is the difference between simulator and xpansion_merged ?

For now, none. I can remove it for better understanding, but the idea was to eventually differentiate both types of problem if needed

tbittar · 2024-02-19T08:54:45Z

src/andromede/simulation/xpansion.py

+
+"""
+The xpansion module extends the optimization module
+with Bender solver related functions


tbittar · 2024-02-19T09:47:14Z

src/andromede/simulation/xpansion.py

+        root_dir = os.getcwd()
+
+        if not os.path.isfile(root_dir + "/bin/benders"):
+            # TODO Maybe a more robust check and/or return value?


For now add an error message to explain the expected binary location

tbittar · 2024-02-19T10:08:37Z

tests/andromede/test_xpansion.py

+
+    output = OutputValues(problem)
+    expected_output = OutputValues()
+    expected_output.component("G1").var("generation").value = 200.0


Check equality with float values with pytest.approx (update everywhere, even in previous unchanged tests)

I'm working on it on another branch with other modifications to the OutputValues class. Stay tuned :)

tbittar · 2024-02-19T10:09:06Z

tests/andromede/test_xpansion.py

+    status = problem.solver.Solve()
+
+    assert status == problem.solver.OPTIMAL
+    assert problem.solver.Objective().Value() == (45 * 200) + (490 * 100 + 10 * 100) + (


Check equality with float values with pytest.approx

tbittar · 2024-02-19T10:09:52Z

tests/andromede/test_xpansion.py

+) -> None:
+    """
+    Same test as before but this time we separate master/subproblem and
+    export the problems in MPS format to be solved by the Bender solver in Xpansion


tbittar · 2024-02-21T07:56:10Z

src/andromede/simulation/xpansion.py

            # TODO Maybe a more robust check and/or return value?
            # For now, it won't look anywhere else because a new
            # architecture should be discussed
+            print(root_dir + "/bin/benders executable not found. Returning True")


In the future you can use f-strings f"{root_dir}/bin/benders executable ..." that is easier to use and more readable when you have more complex strings

Indeed a f-string would be nicer. Since It was a simple string, I didn't bother using one but you are right :)

tbittar · 2024-02-21T08:34:12Z

src/andromede/simulation/optimization.py

-                instantiated_constraint,
-            )
+            for model_var in model.variables.values():
+                if not _should_keep_in_model(self.problem_type, model_var.context):


Much clearer but I still find it even clearer to do if _should_keep_in_model(): even if it makes one more indentation

It would be clarer, from the design point of view, that the choice of "what should be included in the problem" is not hidden deep in the problem building code.
Instead, we could have a small interface that makes this choice, as an entry of build_problem, like:

class ModelSelectionStrategy(ABC): @abstractmethod def get_variables(model: Model) -> List[Variable]: ... @abstractmethod def get_objective(model: Model) -> ExpressionNode: ...

This would have 2 implementations, one for investment, one for operation.
Then you won't need the problem_type argument to build_problem, just this strategy:

master = build_problem( network, database, block, scenarios, problem_name="master", border_management=border_management, solver_id=solver_id, strategy=InvestmentProblemStrategy(), )

tbittar · 2024-02-21T14:01:23Z

tests/andromede/test_xpansion.py


+@pytest.fixture
+def wind_cluster_candidate() -> Model:
+    WIND_CLUSTER_CANDIDATE = model(


Rename this candidate as DISCRETE_CANDIDATE which is more explicit regarding its behaviour and also because usually wind is considered a continuous candidate (whereas gas plant are not for instance)

sylvlecl

A few technical comment + proposition to isolate the particularities of operational and investment problems outside from the build_problem logic, by moving it to a dedicated interface.
This would isolate this logic better.

sylvlecl · 2024-02-21T11:10:24Z

src/andromede/model/common.py

+
+
+class ProblemContext(Enum):
+    operational = 0


Upper case for consistency sake with other parts of the code.

sylvlecl · 2024-02-21T11:17:12Z

src/andromede/model/variable.py

    lower_bound: Optional[ExpressionNode] = None,
    upper_bound: Optional[ExpressionNode] = None,
-    structural_type: Optional[IndexingStructure] = None,
+    structure: IndexingStructure = IndexingStructure(True, True),


Note that it's a little dangerous to use such objects as default in python, because this object will be shared by all calls:
it means that if it's modified at some point, it will be modified for all calls.
However here it's OK because IndexingStructure is immutable (frozen).

I hadn't take it into account, thanks for the heads up!
In this case then you are ok with leaving it like this?

sylvlecl · 2024-02-21T11:17:55Z

src/andromede/simulation/optimization.py

-            if model_var.lower_bound:
-                instantiated_lb_expr = _instantiate_model_expression(
-                    model_var.lower_bound, component.id, opt_context
+        merged = 0


upper case for consistency

Actually this enum could be removed by using an interface for selecting what to include in the problem, see other comment.
It will make this part of the code agnostic from xpansion, which will be clearer.

I am not sure to understand what you meant

sylvlecl · 2024-02-21T12:32:34Z

src/andromede/simulation/optimization.py

+        master = 1
+        subproblem = 2
+
+    name: str


If we don't use dataclass annotation, those fields define class attributes, not instance attributes --> they should be removed since they are actually not used

From what I knew, if you only annotate the type, the interpreter doesn't consider it as a class variable.
It may have changed since, but this post here seems to agree with me.
PEP-526

I created a small snippet to check and it seems to work as I intended

Indeed thanks for sharing the knowledge :)

sylvlecl · 2024-02-21T12:50:16Z

src/andromede/simulation/optimization.py

    database: DataBase,
    block: TimeBlock,
    scenarios: int,
+    *,


I didn't know this syntax!
It forces to use following arguments as keyword arguments (as opposed to positional arguments).
https://peps.python.org/pep-3102/#specification

sylvlecl · 2024-02-21T12:55:25Z

src/andromede/utils.py

    return value
+
+
+def serialize(filename: str, message: str, path: str = "outputs") -> bool:


we can use pathlib.Path for paths, for example it allows to use / operator to have OS-agnostic construction of paths:

file = open(path / filename, "w")

I agree, but since it was just for the PoC, I didn't think we needed it, but I can change it :)

sylvlecl · 2024-02-21T12:57:14Z

src/andromede/utils.py

+        os.makedirs(path, exist_ok=True)
+        file = open(f"{path}/{filename}", "w")
+
+    except os.error:


Why not let the exception pass through ? Do we really want to ignore this error sometimes?
Most of the time it will be better to get an exception instead of ignoring this, otherwise we can get unexpected behaviour afterwards (like xpansion not having one of its input files).

Initially the idea was to catch the return False, but I am not using it this way anyway. I agree that an exception would be better now

sylvlecl · 2024-02-21T14:37:59Z

src/andromede/simulation/optimization.py

-                instantiated_constraint,
-            )
+            for model_var in model.variables.values():
+                if not _should_keep_in_model(self.problem_type, model_var.context):


It would be clarer, from the design point of view, that the choice of "what should be included in the problem" is not hidden deep in the problem building code.
Instead, we could have a small interface that makes this choice, as an entry of build_problem, like:

class ModelSelectionStrategy(ABC): @abstractmethod def get_variables(model: Model) -> List[Variable]: ... @abstractmethod def get_objective(model: Model) -> ExpressionNode: ...

This would have 2 implementations, one for investment, one for operation.
Then you won't need the problem_type argument to build_problem, just this strategy:

master = build_problem( network, database, block, scenarios, problem_name="master", border_management=border_management, solver_id=solver_id, strategy=InvestmentProblemStrategy(), )

sylvlecl · 2024-02-21T14:49:00Z

tests/andromede/test_xpansion.py

+    scenarios = 1
+
+    xpansion = build_xpansion_problem(network, database, TimeBlock(1, [0]), scenarios)
+    assert xpansion.run()


If I understand correctly, it's a little hacky that this passes the CI :)

It would be great to adapt the CI workflow to download the asset from xpansionn, so that it actually runs:
https://github.com/AntaresSimulatorTeam/antares-xpansion/releases/tag/v1.2.1

For example that's what xpansion does for its CI, it downloads antares-solver with a small script step.

We could do that in another PR if you prefer.

You understood it perfectly 😅

I prefer doing it in another PR indeed.
When we create such PR, we could allow it to throw an error if the Benders solvers is not found (then again the question will arise for the other developers that doesn't have it installed locally )

Indeed, maybe we'll need to separate integration tests and unit tests, so that developers don't have to run this one.

sylvlecl · 2024-02-21T14:52:04Z

src/andromede/simulation/xpansion.py

+from andromede.utils import serialize
+
+
+class XpansionProblem:


What about naming it InvestmentProblem, to make reference to the actual feature instead of the existing tool ?

I agree that we could rename it to something not tool related, but since we can solve a investment problem doing a frontal LP, I don't think this name would be very clear.

The main idea here is to separate both master and sub-problem to use the Benders solver. In that case, BendersProblem would be better, no ?

What do you think @tbittar ?

Yes exactly I suggest BendersProblem or DecomposedProblem or BendersDecomposedProblem as we do not want to have any operational knowledge at problem / solver level

The idea here is really that we provide a decomposed form of the big initial problem (and more specially a decomposition suited for a resolution with a Benders algorithm)

Done. I went for BendersDecomposedProblem

tbittar · 2024-02-23T13:07:02Z

src/andromede/simulation/optimization.py

    solver: lp.Solver
    context: OptimizationContext
-    problem_type: Type
+    strategy: Type[ModelSelectionStrategy]


ModelSelectionStrategy is not enough for typing ?

Unfortunately no, because here we are not dealing with instances of the class but the class itself so ModelSelectionStrategy wouldn't pass for MyPy

tbittar · 2024-02-23T13:08:06Z

src/andromede/simulation/optimization.py

        solver: lp.Solver,
        opt_context: OptimizationContext,
-        opt_type: Type = Type.merged,
+        opt_strategy: Type[ModelSelectionStrategy] = MergedProblemStrategy,


build_strategy maybe ?

tbittar · 2024-02-23T14:40:11Z

src/andromede/utils.py


-    except os.error:
-        return False
+    Will throw an exception if it fails to create dir or ro open file


ro is a typo ?

sylvlecl

Thanks, 2 more minor design comments that I'd prefer handled, but not a must-have.

src/andromede/model/model.py

ianmnz added 13 commits February 12, 2024 12:09

See #ANT-958 Create model/common.py and refactor ValueType

7c8a540

See #ANT-958 Add ProblemContext to Par, Var and Constraint

900c776

See #ANT-958 Add ProblemType to build_problem

b678875

See #ANT-958 Add ExportModel to tests

9995811

See #ANT-958 Add Export MPS function for Xpansion's Bender

5464f5d

See #ANT-958 Set verbose name to variables and constraints

87122d1

See #ANT-958 Add cluster_candidate to test

e7aeb39

See #ANT-958 Fixed after merge

12790b0

See #ANT-958 Avoid string parsing for structure.txt

435eee4

See #ANT-958 Rename serialize function

0af3cd8

See #ANT-958 Separate optimization.py from xpansion.py

2997ee7

See #ANT-958 Add node_with_spillage_and_ens to standard models

98acf35

See #ANT-958 Split Xpansion test into two

a56a4b1

ianmnz requested review from sylvlecl and tbittar February 15, 2024 17:18

ianmnz self-assigned this Feb 15, 2024

ianmnz added 2 commits February 15, 2024 18:25

See #ANT-958 To avoid CI errors when ./bender is not found

b4ac1e6

Merge branch 'main' into feature/ANT-958

b30b673

tbittar reviewed Feb 19, 2024

View reviewed changes

See #ANT-958 Fixes after PR review

3d6990c

ianmnz force-pushed the feature/ANT-958 branch from debb29a to 3d6990c Compare February 19, 2024 14:41

See #ANT-958 Add Coupling context for variables

add3654

tbittar reviewed Feb 21, 2024

View reviewed changes

tbittar self-requested a review February 21, 2024 08:00

tbittar approved these changes Feb 21, 2024

View reviewed changes

tbittar reviewed Feb 21, 2024

View reviewed changes

See #ANT-958 String fix after review

eb7a727

tbittar reviewed Feb 21, 2024

View reviewed changes

sylvlecl requested changes Feb 21, 2024

View reviewed changes

See #ANT-958 Rename cluster candidate to better relate to real life

ed94844

ianmnz added 3 commits February 22, 2024 13:18

See #ANT-958 Fixes after review

85c62db

See #ANT-958 Rename Xpansion to BendersDecomposed

635f78d

See #ANT-958 Implement ProblemStrategy interface

8ecaa3d

tbittar reviewed Feb 23, 2024

View reviewed changes

See #ANT-958 Rename build_problem parameter for better understanding

2a26daa

tbittar reviewed Feb 23, 2024

View reviewed changes

tbittar approved these changes Feb 23, 2024

View reviewed changes

sylvlecl approved these changes Feb 26, 2024

View reviewed changes

src/andromede/model/model.py Outdated Show resolved Hide resolved

See #ANT-958 Move ModelSelectionStrategy to simulation directory

07fca4f

tbittar approved these changes Feb 26, 2024

View reviewed changes

sylvlecl merged commit 9fc4464 into main Feb 26, 2024

ianmnz deleted the feature/ANT-958 branch February 28, 2024 12:20

ianmnz mentioned this pull request Mar 13, 2024

Feature/ant 1267 #22

Merged

		return value


		def serialize(filename: str, message: str, path: str = "outputs") -> bool:

Conversation

ianmnz commented Feb 15, 2024

Uh oh!

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!

ianmnz Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

sylvlecl 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!

ianmnz Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

ianmnz Feb 19, 2024 •

edited

Loading

ianmnz Feb 22, 2024 •

edited

Loading

ianmnz Feb 22, 2024 •

edited

Loading