Skip to content

Api thermal heuristic - #238

Open
Juliette-Gerbaux wants to merge 120 commits into
mainfrom
api_thermal_heuristic
Open

Api thermal heuristic#238
Juliette-Gerbaux wants to merge 120 commits into
mainfrom
api_thermal_heuristic

Conversation

@Juliette-Gerbaux

Copy link
Copy Markdown
Contributor

Process ID

Process:

Description

Impact Analysis

Checklist

  • Unit tests pass (pytest)
  • Type checking passes (mypy)
  • Formatting passes (black, isort)
  • pyproject.toml version bumped if applicable
  • AGENTS.md reviewed for impact and updated if needed

Juliette-Gerbaux and others added 23 commits June 17, 2026 10:31
Cross-check each heuristic input/output declared in optim-config.yml against
the model: the referenced id must exist and have the time-dependence the
fast/accurate thermal heuristics expect (e.g. min_up_duration constant,
generation_power per-timestep), catching mismatches at load time instead of
a runtime crash mid-solve. nb_units_max and cluster_max_generation now
accept either form, with the heuristics broadcasting a scalar internally.
Also warn (instead of silently truncating) when min_up_duration/
min_down_duration resolve to a non-integer number of timesteps.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Juliette-Gerbaux
Juliette-Gerbaux marked this pull request as ready for review July 24, 2026 16:53
@Juliette-Gerbaux
Juliette-Gerbaux requested a review from tbittar July 24, 2026 16:54

@tbittar tbittar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may flag all e2e heuristic test so that we are able to deactivate them easily from the CI at some point (if they get too long)

Comment on lines +96 to +97
"nb_units_on_opt",
"nb_units_max",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use the same naming conventions as in the models libraries

Suggested change
"nb_units_on_opt",
"nb_units_max",
"num_units_on_opt",
"num_units_max",

"min_up_duration",
"min_down_duration",
},
"outputs": {"minimum_nb_units_on"},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"outputs": {"minimum_nb_units_on"},
"outputs": {"minimum_num_units_on"},

id: str
model_decomposition: Optional[ModelDecompositionConfig] = None
out_of_bounds_processing: Optional[OutOfBoundsProcessingConfig] = None
heuristic: List[HeuristicConfig] = Field(default_factory=list)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we may have many of them

Suggested change
heuristic: List[HeuristicConfig] = Field(default_factory=list)
heuristics: List[HeuristicConfig] = Field(default_factory=list)

Comment thread src/gems_craft/optim_config/parsing.py Outdated
Comment on lines +648 to +650
v_cfg.id: v_cfg.location
for v_cfg in model_config.model_decomposition.variables
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
v_cfg.id: v_cfg.location
for v_cfg in model_config.model_decomposition.variables
}
variable_config.id: variable_config.location
for variable_config in model_config.model_decomposition.variables
}

Comment on lines +141 to +153
Model on 168 time steps with one thermal generation and one demand on a single node.
- Demand is constant to 2000 MW except for the 13th hour for which it is 2050 MW
- Thermal generation is characterized with:
- P_min = 700 MW
- P_max = 1000 MW
- Min up time = 3
- Min down time = 10
- Generation cost = 50€ / MWh
- Startup cost = 50
- Fixed cost = 1 /h
- Number of unit = 3
- Unsupplied energy = 1000 €/MWh
- Spillage = 0 €/MWh

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expose the system description only once in a comment at the begining of the file

Comment on lines +106 to +121
"num_units_on",
[
0.0,
1.0,
1.0,
1.04,
1.07,
1.11,
1.14,
1.11,
1.08,
1.04,
1.0,
1.0,
0.0,
],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit surprised of this behavior, I would have expected to retrieve the MILP solution of turning on more units (but an integer number of them)

Or maybe this behavior is linked to the addition of ramp constraints ? Is the goal to show the inefficiency of the accurate heuristics in this case ?


def test_accurate_heuristic() -> None:
"""
Check that find_nb_units_accurate produces the same nb_units_on as Antares.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove Antares reference, just say the goal of the test is to retrieve the expected results stored in the repo

# This file is part of the Antares project.

# Expected per-timestep outputs for thermal_heuristic_three_clusters tests.
# Extracted from Antares reference outputs (commit 38c4535).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove reference to Antares, just say these are reference results

return series[:NUMBER_HOURS, SCENARIO].tolist()


def test_accurate_heuristic() -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this file to the unittests directory, then what is the added value of having the previous file testing the fast heuristic ?

@tbittar

tbittar commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Fix solution retrieval, window size in fast heuristic bugs (b96b782)

Follow-up commit addressing 4 issues surfaced by /code-review on this branch:

1. Root cause: solution reads keyed by an ambiguous merged-variable name
For a model that splits an INTEGER/BINARY variable across relaxed and exact integer_strategy groups, optimization.py registers two real linopy.Variables but merges them into a synthetic xr.concat-ed copy for the general linopy_vars lookup dict. That copy's .name arbitrarily reflects only one of the two real registered names. Anything that read solved values via linopy_model.solution[<name>] (heuristic variable-solution inputs, sequential-mode carry-over extraction, and the simulation table's output/extra-output collectors) would silently get NaN or drop the other group's components.

Fixed by adding OptimizationProblem.get_variable_solution(model_id, var_name), which reads .solution directly off the real per-component-registered variables (_linopy_vars_by_component) and reassembles them — the same mechanism already used for safe bound mutation via get_component_variable. Rewired all affected call sites in heuristic_runner.py, session.py, and simulation_table.py.

2. Heuristics/second solve ran before checking the first solve's status
SimulationSession._run_block applied heuristics and re-solved unconditionally before ever checking whether the first solve reached optimality. Moved the status check to run immediately after the first solve (via a small _check_solved helper), so a failed first solve fails fast instead of running the heuristic step against a non-optimal or missing solution.

3. Fast heuristic crashed on zero minimum durations
find_min_generation_fast computed window_size = max(min_up_duration, min_down_duration), which is 0 for a cluster with no minimum up/down time, and used it as a range() step — raising ValueError: range() arg 3 must not be zero. Clamped to max(..., 1), which degenerates correctly to per-timestep processing.

4. Minor: missing space in the solve-failure error message.

Also updated the FakeProblem test doubles in test_simulation_table_{accessor,export,mock}.py to implement get_variable_solution, matching the new interface.

Verification: full suite green (605 passed, 1 xfailed), mypy clean on all touched files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants