Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions temoa/components/capacity.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,11 +331,11 @@ def annual_retirement_constraint(
)
# next v_capacity also accounts for decision retirement so need to undo that again
p_next_end = p_next + value(model.period_length[p_next])
if t in model.tech_retirement and p_next_end <= eol_year:
if t in model.tech_retirement and v < p_next and p_next_end <= eol_year:
cap_end += model.v_retired_capacity[r, p_next, t, v]

# v_capacity already accounts for decision retirement so need to undo that for beginning cap
if t in model.tech_retirement and p_end <= eol_year:
if t in model.tech_retirement and v < p and p_end <= eol_year:
cap_begin += model.v_retired_capacity[r, p, t, v]

annualised_retirement = (cap_begin - cap_end) / value(model.period_length[p])
Expand Down
55 changes: 16 additions & 39 deletions temoa/components/technology.py
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,7 @@ def create_survival_curve(model: TemoaModel) -> None:

# Collect rptv indices into (r, t, v): p dictionary
for r, p, t, v in model.lifetime_survival_curve.sparse_keys():
if (r, t, v) not in model.survival_curve_periods:
model.survival_curve_periods[r, t, v] = set()
model.survival_curve_periods[r, t, v].add(p)
model.survival_curve_periods.setdefault((r, t, v), set()).add(p)
model.is_survival_curve_process[r, t, v] = True

# Go through all the periods for each (r, t, v) in order
Expand All @@ -244,23 +242,21 @@ def create_survival_curve(model: TemoaModel) -> None:

p_first = periods_rtv[0]
p_last = periods_rtv[-1]

if p_first != v:
eol_year = v + value(model.lifetime_process[r, t, v])

if (
p_first != v
or p_last != eol_year
or value(model.lifetime_survival_curve[r, v, t, v]) != 1
or value(model.lifetime_survival_curve[r, eol_year, t, v]) != 0
):
msg = (
'lifetime_survival_curve must be defined starting in the vintage period. Must '
f'define ({r}, >{v}<, {t}, {v})'
'lifetime_survival_curve must be defined as 1 at start and 0 at end of life. Must '
f'define ({r}, >{v}<, {t}, {v}) = 1 and ({r}, >{eol_year}<, {t}, {v}) = 0.'
)
logger.error(msg)
raise ValueError(msg)
Comment on lines +247 to 258
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 8, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Reject zero survival fractions before end-of-life.

This check only validates the endpoints, so a curve that reaches 0 before eol_year and stays there still passes. adjusted_capacity_constraint() later divides v_retired_capacity by lifetime_survival_curve[r, S_p, t, v], so any eligible early-retirement period at or after that first zero still hits the divide-by-zero path this PR is trying to eliminate.

🛠️ Suggested validation hardening
         if (
             p_first != v
             or p_last != eol_year
             or value(model.lifetime_survival_curve[r, v, t, v]) != 1
             or value(model.lifetime_survival_curve[r, eol_year, t, v]) != 0
         ):
             msg = (
                 'lifetime_survival_curve must be defined as 1 at start and 0 at end of life. Must '
                 f'define ({r}, >{v}<, {t}, {v}) = 1 and ({r}, >{eol_year}<, {t}, {v}) = 0.'
             )
             logger.error(msg)
             raise ValueError(msg)
+
+        for p_chk in periods_rtv[:-1]:
+            if value(model.lifetime_survival_curve[r, p_chk, t, v]) <= 0:
+                msg = (
+                    'lifetime_survival_curve must stay strictly positive before end of life. '
+                    f'Found non-positive value at {(r, p_chk, t, v)}.'
+                )
+                logger.error(msg)
+                raise ValueError(msg)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@temoa/components/technology.py` around lines 247 - 258, The current
validation only checks lifetime_survival_curve at the start (p_first) and end
(eol_year) but allows zeros in intermediate years, which later can cause
divide-by-zero in adjusted_capacity_constraint when dividing v_retired_capacity
by lifetime_survival_curve; update the check in the validation block around
lifetime_survival_curve to also iterate all intermediate vintage years between
p_first and eol_year and assert value(model.lifetime_survival_curve[r, y, t, v])
> 0 for each y (or raise the same ValueError with a clear message referencing
lifetime_survival_curve, v_retired_capacity, and adjusted_capacity_constraint)
so any zero before eol_year is rejected.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

lifetime_survival_curve has a default value of zero so it will always be zero outside the lifetime. I don't believe this case would ever come up

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh wait I see okay there is a check that logs as info but should be an error

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

Let me look at the actual constraint to verify whether it already guards against zero survival fractions.


🏁 Script executed:

#!/bin/bash
# Look at adjusted_capacity_constraint and retired_capacity_variable_indices in capacity.py
rg -n "adjusted_capacity_constraint|retired_capacity_variable_indices|lifetime_survival_curve" temoa/components/capacity.py -A 5 -B 2

Length of output: 2771


@idelder, I understand the argument about the default — years outside the lifetime will always be 0 by default, and the constraint conditions (S_p + period_length ≤ eol_year) correctly exclude those. My concern is about years inside the lifetime (strictly between v and eol_year) where the curve could legally reach 0 before eol_year and still pass every current validation check.

The two relevant division sites found in capacity.py are:

Line 322 (inside annual_retirement_constraint):

/ value(model.lifetime_survival_curve[r, p, t, v])

Line 557 (inside adjusted_capacity_constraint):

/ value(model.lifetime_survival_curve[r, S_p, t, v])  # relative survival since then

with the guard v < S_p <= p and S_p + period_length[S_p] <= eol_year.

A concrete scenario that passes all current validation but triggers the divide-by-zero:

Year lifetime_survival_curve[r, year, t, v]
v 1.0
v+3 0.0
v+5 (= eol_year) 0.0
  • p_first == v ✓, p_last == eol_year ✓, starts at 1 ✓, ends at 0 ✓, monotonically non-increasing ✓ — all checks pass.
  • If an optimization period starts at v+3 with period_length = 2, then S_p + period_length = v+5 = eol_year ≤ eol_year — the loop condition is satisfied.
  • Line 557 then divides v_retired_capacity[r, v+3, t, v] by lifetime_survival_curve[r, v+3, t, v] = 0.0ZeroDivisionError.

The proposed fix (rejecting any non-positive value in periods_rtv[:-1]) is the minimal safeguard:

for p_chk in periods_rtv[:-1]:
    if value(model.lifetime_survival_curve[r, p_chk, t, v]) <= 0:
        msg = (
            'lifetime_survival_curve must stay strictly positive before end of life. '
            f'Found non-positive value at {(r, p_chk, t, v)}.'
        )
        logger.error(msg)
        raise ValueError(msg)

This runs after interpolation has filled in all intermediate years, so it catches both explicitly-set zeros and interpolated-to-zero values before the model ever runs.


🧠 Learnings used
Learnt from: idelder
Repo: TemoaProject/temoa PR: 279
File: temoa/components/capacity.py:620-628
Timestamp: 2026-03-17T22:51:03.116Z
Learning: In TemoaProject/temoa `temoa/components/capacity.py`, `create_capacity_and_retirement_sets` must NOT call `value(model.existing_capacity[r, t, v])` as a guard on processes that arrived via `efficiency.sparse_iterkeys()` only, because those processes may have no explicit entry in `existing_capacity` and the access would throw a KeyError/Pyomo error. The upstream `populate_core_dictionaries` in `temoa/components/technology.py` already raises a hard `ValueError` for efficiency-based processes with zero/non-positive existing capacity, so by the time `create_capacity_and_retirement_sets` runs, any `(r, t, v)` in `model.process_periods` with `v in time_exist` is guaranteed to have a valid positive capacity. Do not add a redundant `existing_capacity` guard inside `create_capacity_and_retirement_sets`.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 292
File: temoa/components/capacity.py:335-341
Timestamp: 2026-04-08T19:39:58.846Z
Learning: In TemoaProject/temoa `temoa/components/capacity.py`, `adjusted_capacity_constraint` contains a bug where the early-retirement summation uses a strict `<` guard (`S_p < v + lifetime - period_length[S_p]`, i.e., `S_p_end < eol_year`) instead of `<=`. This means decision retirements in the final pre-EOL period (where `p_end == eol_year`) are never subtracted from `v_capacity`, even though `retired_capacity_variable_indices` allows `v_retired_capacity` to be nonzero for that period (its condition uses non-strict `<=`). The strict `<` guards in `annual_retirement_constraint` (for adding retirement back to cap_begin/cap_end) mask this bug because the two errors cancel out. Both constraints must be fixed together: change both to `<=` so the boundary case is handled consistently.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 279
File: temoa/components/technology.py:163-169
Timestamp: 2026-03-17T21:41:26.527Z
Learning: In TemoaProject/temoa `temoa/components/technology.py`, the two non-positive existing-capacity checks are intentionally different in severity:
1. Inside `populate_core_dictionaries` (iterating over `efficiency.sparse_iterkeys()`): any process that reaches this check has an efficiency entry and would be loaded into the optimization model, where a zero or negative existing capacity causes infeasibility — so a hard `ValueError` is correct.
2. Inside `check_existing_capacity` (iterating over all `existing_capacity.sparse_iterkeys()`): entries here may have no corresponding efficiency entry (non-process existing capacity values) and never enter the model directly — so warn-and-skip (continue) is correct.
Do not flag this difference as an inconsistency; it is intentional.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 277
File: temoa/model_checking/network_model_data.py:357-361
Timestamp: 2026-03-16T18:27:06.688Z
Learning: In TemoaProject/temoa, `NetworkModelData.demand_commodities` in `temoa/model_checking/network_model_data.py` is used only as a starting-node set for the commodity network sweep, not as a validation or filter set. Having extra period keys outside the myopic window is harmless because `available_techs` is already strictly filtered to the active myopic periods; unreachable demand entries are simply ignored during the sweep. Do not flag unfiltered demand_commodities in myopic mode as a correctness issue.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 269
File: temoa/components/technology.py:372-378
Timestamp: 2026-03-09T13:24:27.000Z
Learning: In temoa/components/technology.py, the defunct tech_resource set has been removed. All technologies are now subsets of tech_production, making tech_production the correct reference set for check_efficiency_indices. Do not revert validation back to tech_all; continue using tech_production as the authoritative set.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@idelder, yes — that's exactly the spot. The existing block in the monotonicity loop:

if lsc < 0.0001:
    if p != p_last:
        msg = (
            'There is no need to continue a survival curve beyond fraction ~= 0. '
            f'ignoring periods beyond {p} for ({r, t, v})'
        )
        logger.info(msg)
    continue

needs to become a hard error when p != p_last, because silently continue-ing allows the near-zero value to persist in lifetime_survival_curve for an intermediate year, which is then used as a divisor in the constraints:

if lsc < 0.0001:
    if p != p_last:
        msg = (
            'lifetime_survival_curve must stay strictly positive before end of life. '
            f'Found non-positive value at ({r}, {p}, {t}, {v}). '
            'The survival curve cannot reach ~0 before the end-of-life year.'
        )
        logger.error(msg)
        raise ValueError(msg)
    continue  # p == p_last: expected terminal zero, fine to stop here

The continue branch for p == p_last is correct and can stay as-is — reaching zero exactly at eol_year is the expected terminal condition and is already excluded by the constraint guards. Only the intermediate near-zero case needs to be a ValueError.


🧠 Learnings used
Learnt from: idelder
Repo: TemoaProject/temoa PR: 279
File: temoa/components/capacity.py:620-628
Timestamp: 2026-03-17T22:51:03.116Z
Learning: In TemoaProject/temoa `temoa/components/capacity.py`, `create_capacity_and_retirement_sets` must NOT call `value(model.existing_capacity[r, t, v])` as a guard on processes that arrived via `efficiency.sparse_iterkeys()` only, because those processes may have no explicit entry in `existing_capacity` and the access would throw a KeyError/Pyomo error. The upstream `populate_core_dictionaries` in `temoa/components/technology.py` already raises a hard `ValueError` for efficiency-based processes with zero/non-positive existing capacity, so by the time `create_capacity_and_retirement_sets` runs, any `(r, t, v)` in `model.process_periods` with `v in time_exist` is guaranteed to have a valid positive capacity. Do not add a redundant `existing_capacity` guard inside `create_capacity_and_retirement_sets`.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 292
File: temoa/components/capacity.py:335-341
Timestamp: 2026-04-08T19:39:58.846Z
Learning: In TemoaProject/temoa `temoa/components/capacity.py`, `adjusted_capacity_constraint` contains a bug where the early-retirement summation uses a strict `<` guard (`S_p < v + lifetime - period_length[S_p]`, i.e., `S_p_end < eol_year`) instead of `<=`. This means decision retirements in the final pre-EOL period (where `p_end == eol_year`) are never subtracted from `v_capacity`, even though `retired_capacity_variable_indices` allows `v_retired_capacity` to be nonzero for that period (its condition uses non-strict `<=`). The strict `<` guards in `annual_retirement_constraint` (for adding retirement back to cap_begin/cap_end) mask this bug because the two errors cancel out. Both constraints must be fixed together: change both to `<=` so the boundary case is handled consistently.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 279
File: temoa/components/technology.py:163-169
Timestamp: 2026-03-17T21:41:26.527Z
Learning: In TemoaProject/temoa `temoa/components/technology.py`, the two non-positive existing-capacity checks are intentionally different in severity:
1. Inside `populate_core_dictionaries` (iterating over `efficiency.sparse_iterkeys()`): any process that reaches this check has an efficiency entry and would be loaded into the optimization model, where a zero or negative existing capacity causes infeasibility — so a hard `ValueError` is correct.
2. Inside `check_existing_capacity` (iterating over all `existing_capacity.sparse_iterkeys()`): entries here may have no corresponding efficiency entry (non-process existing capacity values) and never enter the model directly — so warn-and-skip (continue) is correct.
Do not flag this difference as an inconsistency; it is intentional.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 277
File: temoa/model_checking/network_model_data.py:357-361
Timestamp: 2026-03-16T18:27:06.688Z
Learning: In TemoaProject/temoa, `NetworkModelData.demand_commodities` in `temoa/model_checking/network_model_data.py` is used only as a starting-node set for the commodity network sweep, not as a validation or filter set. Having extra period keys outside the myopic window is harmless because `available_techs` is already strictly filtered to the active myopic periods; unreachable demand entries are simply ignored during the sweep. Do not flag unfiltered demand_commodities in myopic mode as a correctness issue.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 281
File: temoa/components/limits.py:509-514
Timestamp: 2026-03-21T00:30:54.385Z
Learning: In TemoaProject/temoa `temoa/components/limits.py`, the `possible_activity_rptvo` RHS sum in `limit_annual_capacity_factor_constraint` intentionally includes capacity for ALL technologies in the expanded tech group (`techs`), regardless of whether each individual tech produces the constrained output commodity `o`. This is by design: the constraint compares activity to total group capacity, and it is the modeller's responsibility to be aware of which technologies contribute capacity to the RHS. Do not flag this unfiltered capacity sum as a bug.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 269
File: temoa/components/technology.py:372-378
Timestamp: 2026-03-09T13:24:27.000Z
Learning: In temoa/components/technology.py, the defunct tech_resource set has been removed. All technologies are now subsets of tech_production, making tech_production the correct reference set for check_efficiency_indices. Do not revert validation back to tech_all; continue using tech_production as the authoritative set.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 277
File: temoa/model_checking/commodity_network_manager.py:129-131
Timestamp: 2026-03-14T02:49:00.990Z
Learning: In the TemoaProject/temoa materials module, for end-of-life (EOL) output flows, the technology is used as the input commodity (`input_comm`). This means `edge_tuple.input_comm` equals the technology name for EOL edges. Consequently, the `rtv_eol` ViableSet in `temoa/model_checking/commodity_network_manager.py` is correctly populated as `(edge_tuple.region, edge_tuple.input_comm, edge_tuple.vintage)` — do not suggest replacing `input_comm` with `tech` here.

Learnt from: idelder
Repo: TemoaProject/temoa PR: 281
File: temoa/data_io/component_manifest.py:610-624
Timestamp: 2026-03-18T19:20:59.326Z
Learning: In TemoaProject/temoa `temoa/data_io/component_manifest.py`, the `validation_map` for share-table LoadItems (`limit_capacity_share`, `limit_new_capacity_share`) intentionally validates only the numerator/sub_group column and NOT the `super_group` (denominator) column. This is a deliberate design decision: forming a viable set covering every possible combination of tech groups would be impractical. Do not flag the absence of `super_group` validation in these share-table LoadItems as a bug or missing validation.


if value(model.lifetime_survival_curve[r, v, t, v]) != 1:
msg_str = (
'lifetime_survival_curve must begin at 1 for calculating annual retirements. '
f'Got {value(model.lifetime_survival_curve[r, v, t, v])} for ({r}, {v}, {t}, {v})'
)
logger.error(msg_str)
raise ValueError(msg_str)

# Collect a list of processes that needed to be interpolated, for warning
if periods_rtv != list(range(p_first, p_last + 1, 1)):
rtv_interpolated.add((r, t, v))
Expand All @@ -283,6 +279,7 @@ def create_survival_curve(model: TemoaModel) -> None:
logger.error(msg)
raise ValueError(msg)

# Linearly interpolate to fill in missing years
if p - p_prev > 1:
_between_periods = [cast('Period', _p) for _p in range(p_prev + 1, p, 1)]
for _p in _between_periods:
Expand All @@ -294,35 +291,15 @@ def create_survival_curve(model: TemoaModel) -> None:
if lsc < 0.0001:
if p != p_last:
msg = (
'There is no need to continue a survival curve beyond fraction ~= 0. '
f'ignoring periods beyond {p} for ({r, t, v})'
)
logger.info(msg)

# Make sure the lifetime for this process aligns with survival curve end
if round(value(model.lifetime_process[r, t, v])) != p - v:
msg = (
f'The lifetime_process parameter '
f'({round(value(model.lifetime_process[r, t, v]))}) for process '
f'{r, t, v} with survival curve does not agree with the end of that '
f'survival curve in {p}. To agree with '
f'the survival curve, set lifetime_process[{r, t, v}] = {p - v}'
'Survival curve must be strictly positive during the '
'lifetime of the process. Found non-positive value at '
f'period {p} for ({r, t, v}) which could cause a '
'division by zero error.'
)
logger.error(msg)
raise ValueError(msg)

continue

# Flag if the last period is not fraction = 0. This is important for investment costs
if p == p_last and lsc > 0.0001:
msg = (
'Any defined survival curve must continue to zero for the purposes of '
'investment cost accounting, even if this period would extend beyond '
f'defined future periods. Continue ({r, t, v}) to fraction == 0.'
)
logger.error(msg)
raise ValueError(msg)

model.survival_curve_periods[r, t, v].update(between_periods)

if rtv_interpolated:
Expand Down
Loading