Clean up capacity index sets#289
Clean up capacity index sets#289ParticularlyPythonicBS merged 6 commits intoTemoaProject:unstablefrom
Conversation
WalkthroughRefactors capacity-related indexing and moves capacity-factor selection into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/components/reserves.py (1)
150-165:⚠️ Potential issue | 🔴 CriticalFix KeyError in
get_capacity_factorwhen called with exchange region keys.The
get_capacity_factorfunction at lines 150 and 161 callsget_capacity_factor(model, r1r2, s, d, t, v)with exchange region keys (e.g.,"REG1-REG2"). However,get_capacity_factordirectly accessesmodel.is_capacity_factor_process[r, t, v]without a fallback. The dictionary is only populated duringcheck_capacity_factor_processfor(r, t, v)tuples where(r, t)appears incapacity_factor_rsdt, which is derived fromactive_capacity_available_rpt. Sinceactive_capacity_available_rptis built fromprocess_vintagesandprocess_vintagescan contain exchange region keys (from efficiency entries with regional_indices), an exchange technology with efficiency but nocapacity_factor_techentry will cause aKeyErrorwhenget_capacity_factoris invoked.Add a
.get()fallback inget_capacity_factorto returncapacity_factor_techas the default when the process is not inis_capacity_factor_process.
🤖 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/capacity.py`:
- Around line 152-155: The nullable return annotations in
new_capacity_variable_indices and capacity_available_variable_indices are
incorrect because TemoaModel.__init__ initializes new_capacity_rtv and
active_capacity_rptv to empty sets; remove the " | None" from both function
return types so they return set[tuple[Region, Technology, Vintage]]
consistently. Locate the functions new_capacity_variable_indices and
capacity_available_variable_indices and change their signatures to return
set[tuple[Region, Technology, Vintage]] (not nullable), keeping the bodies that
return model.new_capacity_rtv and model.active_capacity_rptv respectively.
In `@temoa/components/utils.py`:
- Around line 85-91: In get_capacity_factor, remove the unnecessary else after
the first return to satisfy style linter Ruff RET505: keep the if branch
returning value(model.capacity_factor_process[...]) and then directly return
value(model.capacity_factor_tech[...]) (reference function get_capacity_factor
and the keys model.is_capacity_factor_process, model.capacity_factor_process,
model.capacity_factor_tech).
🪄 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: 9fae0944-7ab2-479c-aa1b-82578ec5288b
📒 Files selected for processing (13)
temoa/components/capacity.pytemoa/components/commodities.pytemoa/components/costs.pytemoa/components/emissions.pytemoa/components/limits.pytemoa/components/operations.pytemoa/components/reserves.pytemoa/components/storage.pytemoa/components/utils.pytemoa/core/model.pytemoa/types/set_types.pytests/legacy_test_values.pytests/test_full_runs.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/components/storage.py (1)
519-523:⚠️ Potential issue | 🟡 MinorRefresh the throughput equation to include the capacity factor.
The implementation now derates storage throughput with
get_capacity_factor(...), but the docstring still documents the oldCAP * C2A * SEGbound. Please update the math block so the code and generated docs describe the same formulation.📝 Suggested doc fix
- \textbf{CAP}_{r,t,v} \cdot C2A_{r,t} \cdot SEG_{s,d} + \text{CF}_{r,s,d,t,v} \cdot \textbf{CAP}_{r,t,v} \cdot C2A_{r,t} \cdot SEG_{s,d}Also applies to: 542-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/components/storage.py` around lines 519 - 523, The docstring math blocks still show the old bound "CAP_{r,t,v} * C2A_{r,t} * SEG_{s,d}" but the code derates throughput using get_capacity_factor(...); update both affected LaTeX/math blocks (the one with FO, FIS, EFF and the other at the later block) to multiply the right-hand side by the capacity factor (e.g., CAP_{r,t,v} * C2A_{r,t} * SEG_{s,d} * CF_{r,t,v} or explicitly call out get_capacity_factor(r,t,v)), keeping the original symbols FO_{...}, FIS_{...}, EFF_{...}, CAP_{...}, C2A_{...}, SEG_{...} unchanged so docs match the implementation.
🤖 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/capacity.py`:
- Around line 648-652: The docstring for create_capacity_and_retirement_sets()
still refers to model.active_capacity_available_rptv while the implementation
populates model.active_capacity_rptv; update the function docstring to mention
model.active_capacity_rptv (and any explanatory text that uses the old name) so
the documentation matches the actual set created by the function and avoid the
old name model.active_capacity_available_rptv.
---
Outside diff comments:
In `@temoa/components/storage.py`:
- Around line 519-523: The docstring math blocks still show the old bound
"CAP_{r,t,v} * C2A_{r,t} * SEG_{s,d}" but the code derates throughput using
get_capacity_factor(...); update both affected LaTeX/math blocks (the one with
FO, FIS, EFF and the other at the later block) to multiply the right-hand side
by the capacity factor (e.g., CAP_{r,t,v} * C2A_{r,t} * SEG_{s,d} * CF_{r,t,v}
or explicitly call out get_capacity_factor(r,t,v)), keeping the original symbols
FO_{...}, FIS_{...}, EFF_{...}, CAP_{...}, C2A_{...}, SEG_{...} unchanged so
docs match the implementation.
🪄 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: 6540de56-6352-4fe7-b89a-69375bf70bc1
📒 Files selected for processing (5)
temoa/components/capacity.pytemoa/components/flows.pytemoa/components/storage.pytemoa/components/technology.pytemoa/components/utils.py
| model.active_capacity_rptv = { | ||
| (r, p, t, v) | ||
| for r, p, t in model.process_vintages | ||
| for r, p, t in model.active_capacity_available_rpt | ||
| for v in model.process_vintages[r, p, t] | ||
| if t not in model.tech_uncap | ||
| } |
There was a problem hiding this comment.
Rename the documented set to active_capacity_rptv.
create_capacity_and_retirement_sets() now populates model.active_capacity_rptv, but the function docstring still advertises model.active_capacity_available_rptv. That leaves the new capacity-index naming self-contradictory right where the rename was introduced.
📝 Suggested doc fix
- - model.active_capacity_available_rptv: set of (r, p, t, v) where vintage capacity is
+ - model.active_capacity_rptv: set of (r, p, t, v) where vintage capacity is🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/components/capacity.py` around lines 648 - 652, The docstring for
create_capacity_and_retirement_sets() still refers to
model.active_capacity_available_rptv while the implementation populates
model.active_capacity_rptv; update the function docstring to mention
model.active_capacity_rptv (and any explanatory text that uses the old name) so
the documentation matches the actual set created by the function and avoid the
old name model.active_capacity_available_rptv.
ParticularlyPythonicBS
merged commit 5627935
into
TemoaProject:unstable
2 participants
The constraint indices sets in components/capacity.py were indented to the centre of the earth. Brought them in line with set comprehension standards laid out in docs.
Also clarified naming of
activity_capacitysets and tidied up their instantiation. Cleaned up the weird "using cost fixed constraint indices for other things" situation. Clarified naming and usage generally.As a result, found that capacity factors for storage were being silently ignored, which I thought were intentionally not supported until I found there are storage capacity factors in utopia. Turned these capacity factors on inside the
storage_throughput_constraint(i.e., charge + discharge flows now constrained by CF, if defined). Made sure all capacity factor calls are using the efficient prepared dictionary lookup.Finally, unified the formatting for declaration of index functions. <--- I think this may need to be reflected in docs after those are updated. As part of this, removed the
| Nonetype for construction sets as they are explicitly declared with= set()so therefore are never none (and this triggers mypy hell where you need to handle the none case everywhere)Summary by CodeRabbit
Refactoring
Behavioral
Chores