Audit and update documentation#286
Open
idelder wants to merge 27 commits intoTemoaProject:unstablefrom
Open
Conversation
Reorganized and updated the discussion of model formulation. Split up the set and parameter mapping tables that show the translation from algebraic expression to database table to model code.
WalkthroughRestructures and expands documentation across many rst files, removes one DB-vs-model comparison doc, and updates constraint/math notation and docstrings in multiple component modules; a few component constraint formulas/indexing were adjusted, but public APIs were not changed. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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/capacity.py (1)
396-403:⚠️ Potential issue | 🟠 MajorDocument the missing
capacity_constraint()branches.Line 402 now scopes
CapacityAnnualto allT^a, butcapacity_annual_constraint_indices()excludestech_demand, and Lines 448-459 still constrain those annual demand technologies via scaledv_flow_out_annual. The adjacentCapacityblock only shows theFO (+ CUR)equality, so neither thatFOAbranch nor the default>=branch appears in the audited formulation.Also applies to: 431-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/components/capacity.py` around lines 396 - 403, The capacity_constraint() implementation is missing branches for the annual-capacity cases noted: update capacity_constraint() to handle the CapacityAnnual index set returned by capacity_annual_constraint_indices() and include the FOA branch (flow-out-annual) and the default >= branch for annual technologies (tech_demand) that were previously excluded; specifically, add logic mirroring the Capacity block's FO (+ CUR) equality for the non-annual case and add a FOA handling path that enforces sum_{I,O} FOA_{r,p,i,t∈T^a,v,o} <= C2A_{r,t} * CAP_{r,p,t,v} (or the intended >= direction per model sign) when the index belongs to CapacityAnnual, and ensure capacity_annual_constraint_indices() and capacity_constraint() use the same membership test so tech_demand entries are consistently included.
🤖 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 334-338: The displayed Limit Activity Share formula currently sums
only FO (mathbf{FO}_{...}) while the implementation includes the annual flow
term v_flow_out_annual (FOA); update the math notation in the Limit Activity
Share expression to either: (a) add the FOA term to both numerator and
denominator (e.g., include \mathbf{FOA}_{r,p,s,d,i,t,v,o} or v_flow_out_annual
in the summed terms alongside \mathbf{FO}), or (b) explicitly restrict the
summation index to t \notin T^a (make clear the scope T^{g_1}, T^{g_2} exclude
T^a) so the displayed formula matches the code that omits annual flows;
reference the LAS_{r,p,g_1,g_2}, \mathbf{FO}_{...}, v_flow_out_annual (FOA) and
T^a/T^{g_1}/T^{g_2} symbols when making the change.
In `@temoa/components/reserves.py`:
- Around line 80-85: The reserve-margin annual-term currently multiplies all
annual FOA generation by DSD_{r,s,d,o}, but per the model the non-demand annual
outputs should use segment_fraction instead; update the reserve equations in
reserves.py so that for FOA terms you use DSD_{r,s,d,o}·FOA only when the output
o is a demand output (i.e., when using demand_specific_distribution), and use
segment_fraction_{r,s,d,o}·FOA for annual outputs that are not demand-serving;
adjust the logic around the FOA/FO terms where demand_specific_distribution and
segment_fraction are applied so both demand and non-demand annual reserve
providers are handled correctly.
---
Outside diff comments:
In `@temoa/components/capacity.py`:
- Around line 396-403: The capacity_constraint() implementation is missing
branches for the annual-capacity cases noted: update capacity_constraint() to
handle the CapacityAnnual index set returned by
capacity_annual_constraint_indices() and include the FOA branch
(flow-out-annual) and the default >= branch for annual technologies
(tech_demand) that were previously excluded; specifically, add logic mirroring
the Capacity block's FO (+ CUR) equality for the non-annual case and add a FOA
handling path that enforces sum_{I,O} FOA_{r,p,i,t∈T^a,v,o} <= C2A_{r,t} *
CAP_{r,p,t,v} (or the intended >= direction per model sign) when the index
belongs to CapacityAnnual, and ensure capacity_annual_constraint_indices() and
capacity_constraint() use the same membership test so tech_demand entries are
consistently included.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3482b223-8d87-4b0d-845b-a30c4bd08386
📒 Files selected for processing (17)
docs/source/Documentation.rstdocs/source/computational_implementation.rstdocs/source/db_model_comparison.rstdocs/source/mathematical_formulation.rstdocs/source/param_desc_and_tables.rstdocs/source/quick_start.rstdocs/source/set_desc_and_tables.rstdocs/source/unit_checking.rstdocs/source/visualization.rsttemoa/components/capacity.pytemoa/components/commodities.pytemoa/components/costs.pytemoa/components/emissions.pytemoa/components/limits.pytemoa/components/operations.pytemoa/components/reserves.pytemoa/components/storage.py
💤 Files with no reviewable changes (1)
- docs/source/db_model_comparison.rst
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/myopic.rst`:
- Around line 111-114: Update the docs to state that last_instance_status may be
None on the first iteration: modify the description for last_instance_status in
docs to mention it can be None (type str | None) in addition to 'optimal' and
'roll_back', and reference the initializer in myopic_sequencer.py where
last_instance_status is set to None; keep the existing text about 'optimal' and
'roll_back' but append a note that on the initial iteration last_instance_status
== None.
- Line 39: Change the self.evolving assignment in the MyopicSequencer class so
it never becomes None by using myopic_options.get('evolving', False) (i.e.,
replace self.evolving: bool = myopic_options.get('evolving') with
myopic_options.get('evolving', False)), and then align the type annotation
accordingly (prefer keeping it as bool if you supply the default; if you
intentionally want to allow None, change the annotation to bool | None instead).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f985fd63-9ffe-4779-9a5e-a2aa609989c7
📒 Files selected for processing (3)
docs/source/myopic.rsttemoa/components/limits.pytemoa/components/reserves.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/computational_implementation.rst`:
- Around line 144-147: The text incorrectly calls the sum(...) usage a "list
comprehension" — it is a generator expression; update the wording around the
example (mentioning sum(...)) to refer to a "generator expression" instead of
"list comprehension", and briefly note that generator expressions produce an
iterator evaluated lazily (unlike list comprehensions which build a list) to
keep the Python terminology precise in the computational_implementation.rst
teaching section.
- Around line 122-123: Update the documentation to use the correct set name
`regions` (not `region`) wherever the text refers to the first index coming from
the set; replace occurrences like :code:`region` with :code:`regions` in the
sentences that describe index origins (e.g., the phrase "the first index comes
from :code:`region`"), and make the same plural correction at the other
occurrence noted (the later paragraph referencing the same index mapping).
Ensure the rest of the set names (`time_optimize`, `commodity_demand`) are left
unchanged.
- Around line 211-233: The docs refer to self.demand_constraint_rpc and
self.demand as if those are the model index names but the actual model uses
self.n for the regions * time_optimize * commodity_demand index and declares
self.demand = Param(self.n); update the narrative and any loader-mapping
examples (including the HybridLoader snippet mentioned) to use the correct
symbol self.n everywhere (and remove or rename references to
demand_constraint_rpc) so the documentation matches the actual TemoaModel
declarations (e.g., adjust the loader mapping lines that currently reference
demand_constraint_rpc to reference self.n and confirm self.demand =
Param(self.n) is shown).
🪄 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: 9c38203e-2084-401b-a251-5948934e0d6a
📒 Files selected for processing (1)
docs/source/computational_implementation.rst
Comment on lines
122
to
+123
| will be an item from :code:`region`, the second from :code:`time_optimize`, | ||
| the third from :code:`time_season`, fourth from :code:`time_of_day`, and the | ||
| fifth from :code:`commodity_demand`. Though one could choose :code:`a`, :code:`b`, | ||
| :code:`c`, :code:`d`, and :code:`e` (or any naming scheme), we chose :code:`p`, :code:`s`, | ||
| :code:`d`, and :code:`dem` as part of a :ref:`naming scheme | ||
| and the third from :code:`commodity_demand`. Though one could choose :code:`a`, |
There was a problem hiding this comment.
Set name should be regions, not region.
The narrative says the first index comes from region, but the model set is regions. Please make this consistent with the actual set naming used throughout the model docs/examples.
Also applies to: 320-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/source/computational_implementation.rst` around lines 122 - 123, Update
the documentation to use the correct set name `regions` (not `region`) wherever
the text refers to the first index coming from the set; replace occurrences like
:code:`region` with :code:`regions` in the sentences that describe index origins
(e.g., the phrase "the first index comes from :code:`region`"), and make the
same plural correction at the other occurrence noted (the later paragraph
referencing the same index mapping). Ensure the rest of the set names
(`time_optimize`, `commodity_demand`) are left unchanged.
There was a problem hiding this comment.
@idelder, understood — I'll leave it as-is. Thanks for the clarification!
🧠 Learnings used
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.
Comment on lines
+144
to
147
| Lines 5 through 9 also showcase a very common idiom in Python: | ||
| list-comprehension. List comprehension is a concise and efficient syntax to | ||
| create lists. As opposed to building a list element-by-element with for-loops, | ||
| list comprehension can convert many statements into a single operation. |
There was a problem hiding this comment.
This is a generator expression, not a list comprehension.
The shown sum(...) form is using a generator expression. Please update wording to keep the Python terminology precise in this teaching section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/source/computational_implementation.rst` around lines 144 - 147, The
text incorrectly calls the sum(...) usage a "list comprehension" — it is a
generator expression; update the wording around the example (mentioning
sum(...)) to refer to a "generator expression" instead of "list comprehension",
and briefly note that generator expressions produce an iterator evaluated lazily
(unlike list comprehensions which build a list) to keep the Python terminology
precise in the computational_implementation.rst teaching section.
There was a problem hiding this comment.
@idelder, no worries — understood! I'll drop this one. Thanks for all the documentation work in this PR.
🧠 Learnings used
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.
Comment on lines
+211
to
+233
| self.demand_constraint_rpc = Set( | ||
| within=self.regions * self.time_optimize * self.commodity_demand | ||
| ) | ||
| self.demand = Param(self.demand_constraint_rpc) | ||
|
|
||
| ... | ||
|
|
||
| self.demand_constraint = Constraint( | ||
| self.demand_constraint_rpc, rule=commodities.demand_constraint | ||
| ) | ||
|
|
||
|
|
||
| As discussed above, the demand_constraint is only valid for certain | ||
| :math:`\{r, p, s, d, dem\}` tuples. Since the modeler can specify the demand | ||
| distribution per commodity (necessary to model demands like heating, that do not | ||
| apply in all time slices), Temoa must ascertain the valid tuples. We have | ||
| implemented this logic in the function :code:`demand_constraint_indices` in | ||
| ``temoa/components/technology.py``. Thus, Line 1 tells Pyomo to instantiate | ||
| :code:`demand_constraint_rpsdc` as a Set of 5-length tuples indices | ||
| (:code:`dimen=5`), and populate it with what Temoa's rule | ||
| :code:`demand_constraint_indices` returns. We omit here an explanation of the | ||
| implementation of the :code:`demand_constraint_indices` function, stating merely | ||
| that it returns the exact indices over which the demand_constraint must to be | ||
| created. With the sparse set :code:`demand_constraint_rpsdc` created, we can now | ||
| can use it in place of the five sets specified in the non-sparse | ||
| :math:`\{r, p, dem\}` tuples. Since not all combinations of region, period, and | ||
| demand commodity may be valid, Temoa must ascertain the valid tuples. | ||
| Thus, Line 1 tells Pyomo to instantiate | ||
| :code:`demand_constraint_rpc` as a Set of 3-length tuple indices | ||
| within the dimensions of the :code:`regions`, :code:`time_optimize`, and | ||
| :code:`commodity_demand` sets, and populate it only with entries loaded from | ||
| the database. Demand values are stored in the parameter :code:`self.demand` declared in line 4. | ||
| Data from the database is loaded into this index set and parameter via the | ||
| :code:`HybridLoader` class in :code:`temoa/data_io/hybrid_loader.py`, declared in | ||
| the :code:`component_manifest.py` file. |
There was a problem hiding this comment.
Use the actual demand index set name from TemoaModel (self.n) in this section.
This section documents self.demand_constraint_rpc, but the current model definition uses self.n for regions * time_optimize * commodity_demand and defines self.demand = Param(self.n). Please align the names here (and in the loader mapping snippet) with temoa/core/model.py to avoid misleading implementation guidance.
Also applies to: 240-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/source/computational_implementation.rst` around lines 211 - 233, The
docs refer to self.demand_constraint_rpc and self.demand as if those are the
model index names but the actual model uses self.n for the regions *
time_optimize * commodity_demand index and declares self.demand = Param(self.n);
update the narrative and any loader-mapping examples (including the HybridLoader
snippet mentioned) to use the correct symbol self.n everywhere (and remove or
rename references to demand_constraint_rpc) so the documentation matches the
actual TemoaModel declarations (e.g., adjust the loader mapping lines that
currently reference demand_constraint_rpc to reference self.n and confirm
self.demand = Param(self.n) is shown).
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the actual model code before saying anything further.
🏁 Script executed:
#!/bin/bash
rg -n "demand_constraint_rpc|demand_constraint_rpsdc|self\.demand\s*=" temoa/core/model.py -A2 -B2
Length of output: 574
@idelder, apologies — my original review comment was incorrect. I checked temoa/core/model.py and the actual definitions are exactly as the documentation states:
self.demand_constraint_rpcis the real index set name (Line 370)self.demand = Param(self.demand_constraint_rpc)is the real parameter declaration (Line 373)
There is no self.n anywhere. The documentation accurately reflects the code. I should not have flagged this. Sorry for the noise!
🧠 Learnings used
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.
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.
Builds on three commits in the doc_mapping branch (rebased)
A gargantuan opus 4.6 assisted audit of documentation.
Summary by CodeRabbit
New Features
check-unitsCLI commandDocumentation
Updates