Fix interaction between survival curves and early retirement#294
Merged
ParticularlyPythonicBS merged 3 commits intoTemoaProject:unstablefrom Apr 8, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR refines validation logic and streamlines code across three components: tightening vintage-based conditions for retirement accounting, consolidating survival curve endpoint validation, and simplifying demand distribution construction by removing unused imports and eliminating a global fill strategy. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/components/technology.py`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 120e411b-c43b-4cb6-a655-aa8a9e419ac9
📒 Files selected for processing (3)
temoa/components/capacity.pytemoa/components/commodities.pytemoa/components/technology.py
Comment on lines
+247
to
258
| 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) |
There was a problem hiding this comment.
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.
Collaborator
Author
There was a problem hiding this comment.
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
Collaborator
Author
There was a problem hiding this comment.
Oh wait I see okay there is a check that logs as info but should be an error
There was a problem hiding this comment.
🧩 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 thenwith 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+3withperiod_length = 2, thenS_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]bylifetime_survival_curve[r, v+3, t, v] = 0.0→ ZeroDivisionError.
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.
There was a problem hiding this comment.
@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)
continueneeds 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 hereThe 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.
idelder
force-pushed
the
fix/survival_curve_retirements
branch
from
78ff2ad to
bd791d0
Compare
ParticularlyPythonicBS
merged commit ec0e2e5
into
TemoaProject:unstable
12 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor