Add configurable demand resolution and DemandActivity mode#274
Open
SutubraResearch wants to merge 1 commit intoTemoaProject:unstablefrom
Open
Add configurable demand resolution and DemandActivity mode#274SutubraResearch wants to merge 1 commit intoTemoaProject:unstablefrom
SutubraResearch wants to merge 1 commit intoTemoaProject:unstablefrom
Conversation
WalkthroughThe main change optimizes demand-activity constraint generation in commodities.py by explicitly handling a special case: single non-annual upstream techs with exactly one input and no annual co-techs. In such cases, flow values are directly fixed and constraint indices are omitted. Supporting changes include formatting updates to Set initialization in model.py and constraint count adjustments in test expectations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
SutubraResearch
force-pushed
the
pr/demand-formulation
branch
3 times, most recently
from
1826e11 to
a95dbfd
Compare
SutubraResearch
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/commodities.py`:
- Around line 292-303: The supply_annual term is being scaled by
model.segment_fraction but the RHS uses model.demand_specific_distribution,
causing inconsistent timeslice scaling; update the calculation that builds
supply_annual (the expression using model.v_flow_out_annual,
model.commodity_up_stream_process, model.tech_annual, and
model.process_inputs_by_output) to multiply by
value(model.demand_specific_distribution[r, p, s, d, dem]) instead of
value(model.segment_fraction[p, s, d, dem]) so the LHS uses the same demand
share as the RHS and matches the other annual-output handling; keep the call to
demand_constraint_timeslice_error_check(supply + supply_annual, r, p, s, d, dem)
unchanged.
- Around line 162-169: The current fast-path collapses vintages by building
distinct_techs = {t for t, v in non_annual} and skipping DemandActivity when
len(distinct_techs) <= 1; change the skip condition to consider vintages so we
only skip when there is at most one non-annual producer (t,v). Update the check
near the non_annual computation in commodities.py (the block that builds
non_annual from model.commodity_up_stream_process and then does the continue) to
either use len(non_annual) <= 1 or create a set of (t,v) pairs and check its
length <= 1, ensuring DemandActivity rows are emitted whenever multiple vintages
exist for the same technology.
In `@temoa/data_io/hybrid_loader.py`:
- Around line 269-274: The code currently passes config.demand_resolution and
config.demand_activity through to _load_component_data without validation; add
explicit validation before those calls: check (self.config.demand_resolution or
'annual') is one of {'annual','timeslice'} and (self.config.demand_activity or
'on') is one of {'on','off'}, and if not raise a clear ValueError (or
ConfigError) mentioning the invalid value and the expected set; only then call
self._load_component_data for model.demand_resolution and
model.demand_activity_mode so downstream branching in components like
temoa/components/flows.py and temoa/components/commodities.py behaves
predictably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07201fc4-4ed2-4625-9227-d517faebaecf
📒 Files selected for processing (11)
temoa/components/commodities.pytemoa/components/flows.pytemoa/core/config.pytemoa/core/model.pytemoa/data_io/hybrid_loader.pytests/legacy_test_values.pytests/test_full_runs.pytests/test_material_results.pytests/testing_data/mediumville_sets.jsontests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.json
💤 Files with no reviewable changes (1)
- tests/legacy_test_values.py
temoa/core/config.py
Outdated
Comment on lines
+52
to
+53
| demand_resolution: str | None = None, | ||
| demand_activity: str | None = None, |
There was a problem hiding this comment.
Validate these new demand-mode switches before storing them.
Unlike scenario_mode, these values are accepted verbatim. Downstream code branches on exact literals, and the branching is asymmetric: a bad demand_resolution can make demand_constraint() take the annual path while demand_constraint_timeslice_indices() still builds timeslice rows. Please keep None as “use default” if you want, but normalize and reject any non-None value outside {annual, timeslice} / {on, off}.
🛡️ Suggested validation
self.save_lp_file = save_lp_file
self.time_sequencing = time_sequencing
self.reserve_margin = reserve_margin
+ if demand_resolution is not None:
+ demand_resolution = demand_resolution.lower()
+ if demand_resolution not in {'annual', 'timeslice'}:
+ raise ValueError(
+ "demand_resolution must be one of {'annual', 'timeslice'}"
+ )
+ if demand_activity is not None:
+ demand_activity = demand_activity.lower()
+ if demand_activity not in {'on', 'off'}:
+ raise ValueError("demand_activity must be one of {'on', 'off'}")
self.demand_resolution = demand_resolution
self.demand_activity = demand_activityAlso applies to: 134-135
temoa/data_io/hybrid_loader.py
Outdated
Comment on lines
+269
to
+274
| self._load_component_data( | ||
| data, model.demand_resolution, [(self.config.demand_resolution or 'annual',)] | ||
| ) | ||
| self._load_component_data( | ||
| data, model.demand_activity_mode, [(self.config.demand_activity or 'on',)] | ||
| ) |
There was a problem hiding this comment.
Reject invalid demand-mode values before loading them.
Lines 269-274 accept any string, but downstream code branches on exact equality. A typo like annnual will not fail here: temoa/components/flows.py falls into the timeslice branch, while temoa/components/commodities.py still leaves the annual demand constraint active and also enables timeslice demand indices. That can change the formulation instead of producing a clear configuration error. Validate both switches here against {'annual', 'timeslice'} and {'on', 'off'} before populating the model sets.
Proposed fix
- self._load_component_data(
- data, model.demand_resolution, [(self.config.demand_resolution or 'annual',)]
- )
- self._load_component_data(
- data, model.demand_activity_mode, [(self.config.demand_activity or 'on',)]
- )
+ demand_resolution = self.config.demand_resolution or 'annual'
+ if demand_resolution not in {'annual', 'timeslice'}:
+ raise ValueError(
+ f"Invalid demand_resolution '{demand_resolution}'. "
+ "Expected 'annual' or 'timeslice'."
+ )
+ self._load_component_data(data, model.demand_resolution, [(demand_resolution,)])
+
+ demand_activity = self.config.demand_activity or 'on'
+ if demand_activity not in {'on', 'off'}:
+ raise ValueError(
+ f"Invalid demand_activity '{demand_activity}'. "
+ "Expected 'on' or 'off'."
+ )
+ self._load_component_data(data, model.demand_activity_mode, [(demand_activity,)])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self._load_component_data( | |
| data, model.demand_resolution, [(self.config.demand_resolution or 'annual',)] | |
| ) | |
| self._load_component_data( | |
| data, model.demand_activity_mode, [(self.config.demand_activity or 'on',)] | |
| ) | |
| demand_resolution = self.config.demand_resolution or 'annual' | |
| if demand_resolution not in {'annual', 'timeslice'}: | |
| raise ValueError( | |
| f"Invalid demand_resolution '{demand_resolution}'. " | |
| "Expected 'annual' or 'timeslice'." | |
| ) | |
| self._load_component_data(data, model.demand_resolution, [(demand_resolution,)]) | |
| demand_activity = self.config.demand_activity or 'on' | |
| if demand_activity not in {'on', 'off'}: | |
| raise ValueError( | |
| f"Invalid demand_activity '{demand_activity}'. " | |
| "Expected 'on' or 'off'." | |
| ) | |
| self._load_component_data(data, model.demand_activity_mode, [(demand_activity,)]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/data_io/hybrid_loader.py` around lines 269 - 274, The code currently
passes config.demand_resolution and config.demand_activity through to
_load_component_data without validation; add explicit validation before those
calls: check (self.config.demand_resolution or 'annual') is one of
{'annual','timeslice'} and (self.config.demand_activity or 'on') is one of
{'on','off'}, and if not raise a clear ValueError (or ConfigError) mentioning
the invalid value and the expected set; only then call self._load_component_data
for model.demand_resolution and model.demand_activity_mode so downstream
branching in components like temoa/components/flows.py and
temoa/components/commodities.py behaves predictably.
a95dbfd to
3903bfc
Compare
SutubraResearch
force-pushed
the
pr/demand-formulation
branch
from
3903bfc to
27dbb14
Compare
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/commodities.py`:
- Around line 111-128: The index generator demand_activity_constraint_indices
currently mutates model state by fixing variables (model.v_flow_out_annual and
model.v_flow_out) while iterating model.demand_constraint_rpc and checking
model.process_inputs_by_output, model.tech_annual, model.demand and
model.demand_specific_distribution; extract that side-effecting logic into a
separate BuildAction (or initialization function) that runs during model
construction and performs the same checks (single non-annual upstream, single
input) and calls .fix(...) on model.v_flow_out_annual[...] and
model.v_flow_out[...] using value(model.demand[...]) and
value(model.demand_specific_distribution[...]) so the original
demand_activity_constraint_indices becomes a pure index-returner (or
alternatively rename it to demand_activity_constraint_indices_and_fix if you
prefer to keep behavior together).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a75c3025-de03-455e-aa51-a9f29f6cd907
📒 Files selected for processing (3)
temoa/components/commodities.pytemoa/core/model.pytests/legacy_test_values.py
Comment on lines
+111
to
+128
| indices = set() | ||
| for r, p, dem in model.demand_constraint_rpc: | ||
| upstream = list(model.commodity_up_stream_process[r, p, dem]) | ||
| non_annual = [(t, v) for t, v in upstream if t not in model.tech_annual] | ||
| has_annual = any(t in model.tech_annual for t, _ in upstream) | ||
| # Single non-annual (t,v) with one input, no annual co-techs: fix variables | ||
| if len(non_annual) == 1 and not has_annual: | ||
| t, v = non_annual[0] | ||
| inputs = list(model.process_inputs_by_output[r, p, t, v, dem]) | ||
| if len(inputs) == 1: | ||
| i = inputs[0] | ||
| dem_val = value(model.demand[r, p, dem]) | ||
| model.v_flow_out_annual[r, p, i, t, v, dem].fix(dem_val) | ||
| for s in model.time_season[p]: | ||
| for d in model.time_of_day: | ||
| dsd = value(model.demand_specific_distribution[r, p, s, d, dem]) | ||
| model.v_flow_out[r, p, s, d, i, t, v, dem].fix(dem_val * dsd) | ||
| continue |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Side effect in index function: acceptable but worth noting.
Fixing variables within demand_activity_constraint_indices is unconventional since the function name suggests it only returns indices. This works correctly because Pyomo's Set(initialize=...) calls the function exactly once during model construction.
For maintainability, consider either:
- Renaming to
demand_activity_constraint_indices_and_fixto reflect the dual purpose, or - Extracting the variable-fixing logic to a separate
BuildAction.
Not blocking since the current approach is functionally correct and achieves the intended constraint reduction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/components/commodities.py` around lines 111 - 128, The index generator
demand_activity_constraint_indices currently mutates model state by fixing
variables (model.v_flow_out_annual and model.v_flow_out) while iterating
model.demand_constraint_rpc and checking model.process_inputs_by_output,
model.tech_annual, model.demand and model.demand_specific_distribution; extract
that side-effecting logic into a separate BuildAction (or initialization
function) that runs during model construction and performs the same checks
(single non-annual upstream, single input) and calls .fix(...) on
model.v_flow_out_annual[...] and model.v_flow_out[...] using
value(model.demand[...]) and value(model.demand_specific_distribution[...]) so
the original demand_activity_constraint_indices becomes a pure index-returner
(or alternatively rename it to demand_activity_constraint_indices_and_fix if you
prefer to keep behavior together).
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
Restructure the demand constraint formulation for significantly better barrier solve performance:
v_flow_outper (region, period, season, day) slice. This eliminates the dense column and improves barrier solve performance by 25-43% on national-scale models.The annual formulation created a dense column in the constraint matrix because every timeslice flow variable for a demand commodity appeared in one summed row. Barrier methods (Cholesky factorization) are sensitive to such dense columns. The timeslice-level formulation produces a sparser matrix with equivalent feasible region.
Files changed
temoa/components/commodities.pytemoa/components/flows.pytech_annualtemoa/core/model.pydemand_constraint_rpsd_demsettests/legacy_test_values.pytests/test_full_runs.pytests/test_material_results.pyPerformance
Tested on a 16-region, 52-week national model:
Test plan
Summary by CodeRabbit
Release Notes
Optimization
Refactor
Tests