Add configurable demand resolution and DemandActivity mode#274
Merged
ParticularlyPythonicBS merged 7 commits intoTemoaProject:unstablefrom Mar 20, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds detection of “singleton” demands (demands with a single upstream producer) that fixes their annual and time-slice flow variables and omits DemandActivity constraint generation for those demands; registers a BuildAction to run this check during model construction and updates tests/data accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as ModelBuilder
participant Commod as commodities.check_singleton_demands
participant Model as PyomoModel
participant DAC as demand_constraint/DemandActivity
Builder->>Model: instantiate model, register BuildActions
Builder->>Commod: invoke check_singleton_demands(model)
Commod->>Model: scan demand_constraint_rpc indices
alt single upstream found for (r,p,dem)
Commod->>Model: set model.v_flow_out_annual[r,p,i,t,v,dem] = demand
Commod->>Model: set model.v_flow_out[...time-slices...] = demand * distribution
Commod->>Model: add (r,p,dem) to model.singleton_demands
end
Builder->>DAC: trigger demand_constraint build
DAC->>Model: for each (r,p,dem) check if in singleton_demands
alt in singleton_demands
DAC-->>Model: Constraint.Skip (omit DAC and error check)
else
DAC-->>Model: build DemandActivity indices and demand_constraint as usual
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
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
Pr/demand formulation
ParticularlyPythonicBS
merged commit 9773eff
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
3 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
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
Optimization
Refactor
Tests
Documentation