Add economies of scale extension#349
Open
idelder wants to merge 15 commits into
Open
Conversation
WalkthroughThis PR introduces optional extension support, adds discrete-capacity, growth-rate, EOS, and template extensions, migrates related data loading and schema handling, refactors cost polling, updates myopic and tutorial database flows, and expands documentation and tests. ChangesExtension framework and modeling features
Core data and validation
Documentation and cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
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)
926-937: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
vintageforeign key definition.The
vintagecolumn has an inline FK at line 926 (REFERENCES time_period (period)) and a duplicate table-level FK at line 937 (FOREIGN KEY (vintage) REFERENCES time_period (period)). Since this line was already modified to remove the tech FK, this is a good opportunity to drop the redundant table-level vintage FK as well.♻️ Proposed cleanup
PRIMARY KEY (scenario, region, period, tech, vintage), - FOREIGN KEY (vintage) REFERENCES time_period (period) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/db_schema/temoa_schema_v4.sql` around lines 926 - 937, The schema still defines `vintage` twice as a foreign key in `temoa_schema_v4.sql`, once inline on the `vintage` column and again in the table constraint. Remove the redundant table-level `FOREIGN KEY (vintage) REFERENCES time_period (period)` from the same CREATE TABLE block that includes `PRIMARY KEY (scenario, region, period, tech, vintage)`, while keeping the inline `vintage INTEGER REFERENCES time_period (period)` definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/extensions/discrete_capacity.rst`:
- Around line 47-56: Update the `limit_discrete_capacity` documentation to
explicitly warn that it constrains total retirement-adjusted available capacity,
including existing fleet capacity, not just new builds. Add a brief caveat near
the `limit_discrete_capacity` description stating that the unit size must evenly
divide any pre-existing capacity for the affected technology/group, otherwise
the model may become infeasible. Refer to the `limit_discrete_capacity` and
`limit_discrete_new_capacity` sections to clearly distinguish the two behaviors.
In `@scripts/generate_etl_figure.py`:
- Around line 112-117: The mypy failures come from `dash_kw` in
`generate_etl_figure.py` being inferred as `dict[str, object]`, which breaks the
`ax.plot()` `**kwargs` calls inside the `segments` loop. Update the `dash_kw`
definition to use an explicit `dict[str, Any]` annotation so the keyword
arguments match `matplotlib`’s overloaded `ax.plot` signature and the four plot
calls in that loop type-check cleanly.
In `@temoa/_internal/run_actions.py`:
- Around line 247-250: The dual suffix handling in the solver branch needs to be
made safe and use the public API. In the `run_actions` logic where `solver_name
== 'appsi_highs'`, guard the result of `instance.component('dual')` before using
it since it may be `None`, and update the suffix direction via the public
`direction` property on `dual_suffix` instead of assigning to the private
`_direction` field.
In `@temoa/cli.py`:
- Around line 587-601: Extract the duplicated database निर्माण flow in
temoa/cli.py into a shared helper such as
_build_tutorial_database(target_database, schema_sql, tutorial_sql), and move
the common sequence from the current production path and fallback path there:
connect, execute schema, temporarily disable foreign keys, load tutorial SQL,
re-enable foreign keys, run PRAGMA foreign_key_check, and commit. Update both
callers to only read their respective SQL sources and delegate to the helper,
keeping the fallback IntegrityError behavior with from None unchanged.
In `@temoa/components/technology.py`:
- Around line 451-459: In the existing-capacity handling branch in
technology-related processing, replace the warning-only path with a hard
validation failure when surviving_cap is positive and (r, t, v) is not in
model.process_periods. Keep any myopic tolerance behavior only behind an
explicit mode/threshold check, and use the same logger context around the
lifetime_survival_curve and process_periods check to raise a ValueError instead
of continuing.
In `@temoa/components/utils.py`:
- Around line 105-109: The capacity_adjustment calculation in the helper
currently sums retired_existing_capacity for every model.time_exist period,
which can include periods at or before the vintage and lead to incorrect
capacity subtraction. Update the logic in the capacity_adjustment expression to
only include retired_existing_capacity entries for post-vintage periods,
matching the same vintage check used by adjusted_capacity_constraint and the
capacity formula, and keep the lifetime_survival_curve denominator handling
unchanged.
In `@temoa/core/model.py`:
- Around line 42-45: The core model is directly importing and calling the
EOS-specific early registration hook, which creates an unnecessary hard
dependency on a single extension. Remove the `register_early_eos_components`
import from `temoa.core.model` and move early registration behind the generic
extension framework by adding an optional early hook (for example on
`ExtensionSpec`) and dispatching it from `apply_model_extension_hooks` or a new
`apply_early_model_extension_hooks`. Update the core model call site to iterate
enabled extensions and invoke the generic early hook only when present, so the
EOS logic is loaded only if `self.enabled_extensions` includes it.
In `@temoa/data_io/hybrid_loader.py`:
- Around line 697-764: The custom loaders for lifetime tables are querying
optional tables directly, which can still raise OperationalError on older
schemas. Update _load_lifetime_tech, _load_lifetime_process, and
_load_lifetime_survival_curve in HybridLoader to follow the same optional-table
guard used elsewhere before running the SELECT. If the table is absent, skip
loading instead of re-querying it, while keeping the existing filtering logic
for viable_rt, viable_rtv, and myopic_index.
In `@temoa/extensions/economies_of_scale/components/cost_fixed_eos.py`:
- Around line 37-60: The fixed-cost EOS validation in cost_fixed_eos.py needs an
extra guard that the first segment starts at zero. Update the segment checks in
the loop over model.cost_fixed_eos_segments (using the cap_lower/cap_upper and
cost_lower/cost_upper values from model.cost_fixed_eos[r, p, t, n]) so the
earliest segment for each (r, p, t) requires cap_lower == 0 and cost_lower == 0
before proceeding. Keep the existing nonnegative and increasing-bound checks,
and raise the same ValueError/log path when the first segment violates this
zero-origin requirement.
In `@temoa/extensions/economies_of_scale/components/cost_invest_eos.py`:
- Around line 73-96: The validation in cost_invest_eos should also ensure the
first investment segment starts at zero, since a positive cap_lower or
cost_lower on the initial segment makes zero deployment infeasible and
pre-charges cost; update the guard in the loop over
model.cost_invest_eos_segments to special-case the first sorted segment for each
(r, t) and require its lower capacity and cost bounds to be zero, while keeping
the existing nonnegative and increasing checks for all segments.
In `@temoa/extensions/economies_of_scale/components/cost_variable_eos.py`:
- Around line 37-60: The validation in cost_variable_eos.py only checks for
nonnegative, increasing segment bounds, but it must also ensure the first
segment starts at zero. Update the guard in the loop over
model.cost_variable_eos_segments (and the cost_variable_eos table lookup) so the
segment with the lowest index for each r, p, t requires both activity_lower and
cost_lower to be 0. Keep the existing monotonicity checks for the other bounds,
and raise the same ValueError with a clear message if the first segment is not
anchored at zero.
In `@temoa/extensions/economies_of_scale/core/data_puller.py`:
- Around line 101-107: The EOS cost accumulation in the data_puller logic
assumes entries[key] already exists and that .update() can safely add EOS
values, which can fail for replacement cases and overwrite existing flat costs.
Update the accumulation flow in the affected branches of the EOS cost handling
code so it safely initializes or fetches the per-key entry before adding to
CostType.D_INVEST and CostType.INVEST, and merge EOS costs without replacing any
existing base fixed/variable cost fields. Apply the same fix consistently in the
repeated accumulation paths in the data_puller module.
- Around line 149-150: The EOS variable-cost extraction in data_puller.py is
iterating the fixed cluster set instead of the variable period set, so update
the loop in the period-cost collection logic to use the variable EOS rows source
and keep cost_variable_eos.period_cost() aligned with variable-only clusters.
Locate the iteration around the cost extraction that currently walks
model.cost_fixed_eos_period_rpt and switch it to the corresponding variable EOS
period set so fixed-only clusters are not passed through and variable-only costs
are included in the output.
In `@temoa/extensions/economies_of_scale/core/model.py`:
- Around line 142-170: The fixed EOS cost component is defined and constrained
in model setup, but its objective contribution is never registered with the
model. Update the setup around initialize_cost_fixed_eos and
cost_fixed_eos.total_cost() so the total fixed EOS cost is attached as a
BuildAction and incorporated into model.total_cost, using the existing
cost_fixed_eos module and the model initialization block to place the
registration alongside the other EOS component hooks.
In `@temoa/extensions/framework.py`:
- Around line 119-141: The guard in assert_disabled_extension_tables_are_empty
currently only logs a warning when disabled extension-owned tables contain rows,
but it should fail fast instead. Update the populated-table branch in
assert_disabled_extension_tables_are_empty to raise an exception rather than
calling logger.warning, keeping the existing message content so callers cannot
proceed with disabled extension data silently ignored.
In `@temoa/extensions/growth_rates/components/growth_new_capacity_delta.py`:
- Around line 78-88: Update the docstring in growth_new_capacity_delta to use
the correct Theta set names: replace the growth and degrowth references from
limit_growth_capacityDelta and limit_degrowth_capacityDelta to
limit_growth_new_capacity_delta and limit_degrowth_new_capacity_delta. Keep the
terminology consistent with the surrounding function, parameter, and constraint
names in growth_new_capacity_delta and the model registration so the documented
math matches the implemented identifiers.
In `@temoa/extensions/growth_rates/components/growth_new_capacity.py`:
- Around line 76-84: The docstring in growth_new_capacity should use the correct
Theta set names for this module. Update the two copied references from
growth_capacity.py so they match the growth_new_capacity and
degrowth_new_capacity parameter sets used by the corresponding constraint logic,
keeping the rest of the math text unchanged.
In `@temoa/extensions/method_of_morris/morris.py`:
- Line 50: The in-function import of contextlib is redundant because contextlib
is already imported at the module level. Remove the extra import from the
function that contains it in morris.py, keeping only the module-level import so
the function body stays clean and consistent with the rest of the file.
In `@temoa/extensions/monte_carlo/mc_sequencer.py`:
- Around line 153-156: Remove the commented-out kwargs dead code in MCSequencer:
the unused commented dictionary block in mc_sequencer.py should be deleted since
MCWorker already takes solver_name and solver_options directly in its
constructor. Update the surrounding code near the MCSequencer logic to keep only
the active argument passing and eliminate the stale commented lines.
In `@temoa/tutorial_assets/config_sample.toml`:
- Around line 21-25: The sample in config_sample.toml uses an extension ID that
is not registered, so the example can trigger an unknown-extension config error.
Update the example extension list to use only IDs returned by
get_known_extension_specs(), and adjust the surrounding comment/example text so
it only shows registered extension names for the extensions setting.
---
Outside diff comments:
In `@temoa/db_schema/temoa_schema_v4.sql`:
- Around line 926-937: The schema still defines `vintage` twice as a foreign key
in `temoa_schema_v4.sql`, once inline on the `vintage` column and again in the
table constraint. Remove the redundant table-level `FOREIGN KEY (vintage)
REFERENCES time_period (period)` from the same CREATE TABLE block that includes
`PRIMARY KEY (scenario, region, period, tech, vintage)`, while keeping the
inline `vintage INTEGER REFERENCES time_period (period)` definition.
🪄 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: 5e28e099-879c-4bc2-b2ef-e8e8e93e2589
⛔ Files ignored due to path filters (4)
docs/source/images/eos_cost_curve.pngis excluded by!**/*.pngdocs/source/images/eos_cost_curve.svgis excluded by!**/*.svgdocs/source/images/etl_cost_curve.pngis excluded by!**/*.pngdocs/source/images/etl_cost_curve.svgis excluded by!**/*.svg
📒 Files selected for processing (105)
.pre-commit-config.yamldocs/source/computational_implementation.rstdocs/source/extensions.rstdocs/source/extensions/discrete_capacity.rstdocs/source/extensions/eos.rstdocs/source/extensions/growth_rates.rstdocs/source/index.rstdocs/source/mathematical_formulation.rstdocs/source/param_desc_and_tables.rstpyproject.tomlscripts/generate_etl_figure.pytemoa/__init__.pytemoa/_internal/exchange_tech_cost_ledger.pytemoa/_internal/run_actions.pytemoa/_internal/table_data_puller.pytemoa/_internal/temoa_sequencer.pytemoa/cli.pytemoa/components/capacity.pytemoa/components/costs.pytemoa/components/limits.pytemoa/components/technology.pytemoa/components/time.pytemoa/components/utils.pytemoa/core/config.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/data_io/loader_manifest.pytemoa/db_schema/temoa_schema_v4.sqltemoa/extensions/discrete_capacity/__init__.pytemoa/extensions/discrete_capacity/components/__init__.pytemoa/extensions/discrete_capacity/components/discrete_capacity.pytemoa/extensions/discrete_capacity/core/__init__.pytemoa/extensions/discrete_capacity/core/model.pytemoa/extensions/discrete_capacity/data_manifest.pytemoa/extensions/discrete_capacity/extension.pytemoa/extensions/discrete_capacity/tables.sqltemoa/extensions/economies_of_scale/__init__.pytemoa/extensions/economies_of_scale/components/__init__.pytemoa/extensions/economies_of_scale/components/cost_fixed_eos.pytemoa/extensions/economies_of_scale/components/cost_invest_eos.pytemoa/extensions/economies_of_scale/components/cost_variable_eos.pytemoa/extensions/economies_of_scale/core/__init__.pytemoa/extensions/economies_of_scale/core/data_puller.pytemoa/extensions/economies_of_scale/core/model.pytemoa/extensions/economies_of_scale/data_manifest.pytemoa/extensions/economies_of_scale/extension.pytemoa/extensions/economies_of_scale/tables.sqltemoa/extensions/framework.pytemoa/extensions/get_comm_tech.pytemoa/extensions/growth_rates/__init__.pytemoa/extensions/growth_rates/components/__init__.pytemoa/extensions/growth_rates/components/growth_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity_delta.pytemoa/extensions/growth_rates/core/__init__.pytemoa/extensions/growth_rates/core/model.pytemoa/extensions/growth_rates/data_manifest.pytemoa/extensions/growth_rates/extension.pytemoa/extensions/growth_rates/tables.sqltemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/method_of_morris/morris_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/hull.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/example_builds/scenario_analyzer.pytemoa/extensions/monte_carlo/example_builds/scenario_maker.pytemoa/extensions/monte_carlo/mc_run.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/myopic/evolution_updater.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytemoa/extensions/template/__init__.pytemoa/extensions/template/components/__init__.pytemoa/extensions/template/components/example_limit.pytemoa/extensions/template/core/__init__.pytemoa/extensions/template/core/model.pytemoa/extensions/template/data_manifest.pytemoa/extensions/template/extension.pytemoa/extensions/template/tables.sqltemoa/tutorial_assets/config_sample.tomltemoa/tutorial_assets/utopia.sqltests/conftest.pytests/legacy_test_values.pytests/test_cli.pytests/test_extensions.pytests/test_full_runs.pytests/test_table_writer.pytests/testing_configs/config_myopic_capacities.tomltests/testing_data/mediumville.sqltests/testing_data/mediumville_sets.jsontests/testing_data/myopic_capacities.sqltests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.jsontests/testing_data/utopia_v3.sql
💤 Files with no reviewable changes (5)
- temoa/extensions/modeling_to_generate_alternatives/manager_factory.py
- tests/testing_data/mediumville.sql
- docs/source/param_desc_and_tables.rst
- docs/source/mathematical_formulation.rst
- temoa/extensions/stochastics/stochastic_config.py
idelder
force-pushed
the
extension/economies_of_scale
branch
from
July 8, 2026 21:49
09e9003 to
7afdefd
Compare
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/extensions/method_of_morris/morris_evaluate.py (1)
85-87: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
if not status:never triggers —statusis a tuple (always truthy).
run_actions.check_solve_status()returnstuple[bool, str]. A non-empty tuple is always truthy in Python, soif not status:is alwaysFalse— theRuntimeErroris never raised, even for non-optimal solves. This is pre-existing, but now more relevant sinceextensions=config.extensions(line 80) activates the EOS extension in Method of Morris runs, adding solver complexity that could produce non-optimal terminations.Proposed fix
- status = run_actions.check_solve_status(res) - if not status: - raise RuntimeError('Bad solve during Method of Morris') + ok, msg = run_actions.check_solve_status(res) + if not ok: + raise RuntimeError(f'Bad solve during Method of Morris: {msg}')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/method_of_morris/morris_evaluate.py` around lines 85 - 87, The Method of Morris solve check in morris_evaluate.py is testing the whole return value from run_actions.check_solve_status(), but that function returns a tuple[bool, str] so the condition never fails. Update the status handling around check_solve_status and the subsequent RuntimeError in the Method of Morris evaluation flow to inspect the boolean element explicitly and raise when the solve is not acceptable, using the existing res/status variables in morris_evaluate rather than the tuple object itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_full_runs.py`:
- Around line 184-192: The assertions in the `output_cost` queries are using the
raw cost fields instead of the discounted ones, so update the SQL in this test
to read from the `d_*` columns. In the `test_full_runs` checks for
`tech_discrete_cap` and `tech_discrete_new_cap`, replace the `fixed`, `var`, and
`invest` sums with `d_fixed`, `d_var`, and `d_invest` so the values match the
“discounted” wording used in the test expectations.
---
Outside diff comments:
In `@temoa/extensions/method_of_morris/morris_evaluate.py`:
- Around line 85-87: The Method of Morris solve check in morris_evaluate.py is
testing the whole return value from run_actions.check_solve_status(), but that
function returns a tuple[bool, str] so the condition never fails. Update the
status handling around check_solve_status and the subsequent RuntimeError in the
Method of Morris evaluation flow to inspect the boolean element explicitly and
raise when the solve is not acceptable, using the existing res/status variables
in morris_evaluate rather than the tuple object itself.
🪄 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: 380e402b-f9a6-4103-816c-f6f57a1b8851
📒 Files selected for processing (20)
scripts/generate_etl_figure.pytemoa/_internal/run_actions.pytemoa/extensions/discrete_capacity/core/model.pytemoa/extensions/economies_of_scale/core/data_puller.pytemoa/extensions/economies_of_scale/core/model.pytemoa/extensions/get_comm_tech.pytemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/template/core/model.pytests/test_full_runs.pytests/test_table_writer.pytests/testing_configs/config_myopic_capacities.tomltests/testing_data/myopic_capacities.sql
💤 Files with no reviewable changes (4)
- temoa/extensions/modeling_to_generate_alternatives/manager_factory.py
- temoa/extensions/stochastics/stochastic_config.py
- temoa/extensions/discrete_capacity/core/model.py
- temoa/extensions/method_of_morris/morris.py
idelder
force-pushed
the
extension/economies_of_scale
branch
from
July 9, 2026 01:00
9bc80fd to
04f9a16
Compare
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
…P models Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
Signed-off-by: Davey Elder <iandavidelder@gmail.com>
idelder
force-pushed
the
extension/economies_of_scale
branch
from
July 14, 2026 14:54
35e928b to
0ce8cc0
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
temoa/extensions/get_comm_tech.py (1)
23-41: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winEnsure the SQLite connection is closed on exception.
If an exception occurs during the database queries, the connection will not be properly closed, leading to a resource leak. Based on learnings, when working with SQLite in this codebase, prefer wrapping the connection usage in a
try/finallyblock (orcontextlib.closing) to guarantee it is closed on exit.🛡️ Proposed fix to ensure the connection is closed safely
con = sqlite3.connect(inp_f) - cur = con.cursor() # a database cursor is a control structure that enables traversal over - # the records in a database - con.text_factory = str # this ensures data is explored with the correct UTF-8 encoding - - print(inp_f) - cur.execute('SELECT DISTINCT scenario FROM output_flow_out') - x = [] - for row in cur: - x.append(row[0]) - for y in x: - cur.execute('SELECT DISTINCT period FROM output_flow_out WHERE scenario = ?', (y,)) - periods_list[y] = [] - for per in cur: - z = per[0] - periods_list[y].append(z) - - cur.close() - con.close() + try: + cur = con.cursor() # a database cursor is a control structure that enables traversal over + # the records in a database + con.text_factory = str # this ensures data is explored with the correct UTF-8 encoding + + print(inp_f) + cur.execute('SELECT DISTINCT scenario FROM output_flow_out') + x = [] + for row in cur: + x.append(row[0]) + for y in x: + cur.execute('SELECT DISTINCT period FROM output_flow_out WHERE scenario = ?', (y,)) + periods_list[y] = [] + for per in cur: + z = per[0] + periods_list[y].append(z) + + cur.close() + finally: + con.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/get_comm_tech.py` around lines 23 - 41, Wrap the SQLite operations in the connection-using block around con, cur, and the scenario/period queries in a try/finally structure so con.close() executes even when a query raises; keep cursor cleanup and existing query behavior unchanged.Source: Learnings
temoa/db_schema/temoa_schema_v4.sql (1)
919-938: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRestore the
output_cost.techforeign key.output_costis still written fromTechnologykeys, and downstream export joinsoc.techtoTechnology.tech, so dropping this constraint removes a real integrity check rather than supporting EOS group names.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/db_schema/temoa_schema_v4.sql` around lines 919 - 938, Restore the foreign key constraint for output_cost.tech in the output_cost table definition, referencing the Technology.tech key used by Technology writes and downstream joins. Leave the existing columns and primary key unchanged.
♻️ Duplicate comments (2)
temoa/extensions/economies_of_scale/components/cost_variable_eos.py (1)
37-83: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFirst
cost_variable_eossegment is not required to start at zero activity/cost — unresolved from prior review.
initialize_componentsvalidates nonnegativity, monotonic bounds, and inter-segment alignment fori > 0, but never checks that the lowest-indexed segment (i == 0) hasact_lower == 0andcost_lower == 0. Sincecost_variable_eos_segment_binary_constraintforces exactly one segment to always be active, and the lower-bound constraint tiesv_cost_variable_eos_activity >= binary * act_lower, an unanchored first segment forces the technology to always have nonzero activity (and incur nonzero cost) even when it should legitimately be idle.This was already flagged in a prior review with a suggested fix that guards
i == 0foract_lower/cost_lowerbeing (near) zero; that guard is still absent in this version.🎯 Previously suggested fix
for i, n in enumerate(sorted_segs): act_lower, act_upper, cost_lower, cost_upper = model.cost_variable_eos[r, p, t, n] + + if i == 0 and (abs(act_lower) > 0.001 or abs(cost_lower) > 0.001): + msg = ( + 'The first cost_variable_eos segment must start at zero activity ' + f'and zero cost for {r, p, t, n}' + ) + logger.error(msg) + raise ValueError(msg) # Check cost curve is nonnegative and monotonically increasing🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/extensions/economies_of_scale/components/cost_variable_eos.py` around lines 37 - 83, Update initialize_components so the first sorted segment in model.cost_variable_eos_segments, identified by i == 0, validates that act_lower and cost_lower are approximately zero before continuing. Preserve the existing nonnegativity and monotonicity checks, and raise the same validation error path when the first segment is unanchored.temoa/cli.py (1)
587-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated schema-first database generation logic into a helper.
The production path (lines 587–601) and fallback path (lines 642–655) contain near-identical logic: execute schema, disable FK, load data, re-enable FK, run
foreign_key_check, commit. The only differences are the source of the SQL strings and thefrom Noneon the fallbackIntegrityError. Extracting a_build_tutorial_database(target_database, schema_sql, tutorial_sql)helper eliminates the duplication and ensures both paths stay in sync if the generation logic evolves.♻️ Proposed refactor
+def _build_tutorial_database( + target_database: Path, schema_sql: str, tutorial_sql: str +) -> None: + """Create a SQLite database from canonical schema + tutorial data, validating FK integrity.""" + import contextlib + import sqlite3 + + if target_database.exists(): + target_database.unlink() + + with contextlib.closing(sqlite3.connect(target_database)) as conn: + conn.executescript(schema_sql) + conn.execute('PRAGMA foreign_keys = OFF;') + conn.executescript(tutorial_sql) + conn.execute('PRAGMA foreign_keys = ON;') + fk_violations = conn.execute('PRAGMA foreign_key_check;').fetchall() + if fk_violations: + raise sqlite3.IntegrityError( + f'Foreign key check failed after tutorial data load: {fk_violations[:5]}' + ) + conn.commit() + def _copy_tutorial_resources(target_config: Path, target_database: Path) -> None: ... - # Generate database from canonical schema and tutorial data source - schema_content = schema_resource.read_text(encoding='utf-8') - sql_content = sql_resource.read_text(encoding='utf-8') - - with contextlib.closing(sqlite3.connect(target_database)) as conn: - conn.executescript(schema_content) - conn.execute('PRAGMA foreign_keys = OFF;') - conn.executescript(sql_content) - conn.execute('PRAGMA foreign_keys = ON;') - fk_violations = conn.execute('PRAGMA foreign_key_check;').fetchall() - if fk_violations: - raise sqlite3.IntegrityError( - f'Foreign key check failed after tutorial data load: {fk_violations[:5]}' - ) - conn.commit() + _build_tutorial_database( + target_database, + schema_resource.read_text(encoding='utf-8'), + sql_resource.read_text(encoding='utf-8'), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@temoa/cli.py` around lines 587 - 601, Extract the shared schema-first generation sequence from the production and fallback paths into a _build_tutorial_database(target_database, schema_sql, tutorial_sql) helper. Move connection setup, schema/data execution, foreign-key toggling and validation, violation handling, and commit into that helper, then have both paths call it with their respective SQL sources while preserving the fallback path’s existing from None behavior for IntegrityError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@temoa/cli.py`:
- Around line 642-655: Extract the schema-first database generation sequence
from the current command flow into a reusable helper, including schema
execution, SQL data loading, foreign-key validation, and commit behavior. Update
this block and the corresponding logic around the earlier database-generation
section to call the helper, preserving the existing target_database,
fallback_schema, and fallback_sql inputs and IntegrityError behavior.
In `@temoa/data_io/hybrid_loader.py`:
- Around line 249-252: Update the condition in the raw-data reshaping block to
check whether item.index_length is not None rather than relying on truthiness,
while retaining the existing column-length constraint and reshaping behavior for
index_length=0.
In `@temoa/extensions/discrete_capacity/components/discrete_capacity.py`:
- Around line 8-77: Clean up the imports by removing the redundant geography and
technology aliases and update their usages to the direct module references. In
limit_discrete_new_capacity_indices and limit_discrete_capacity_indices, return
each set comprehension directly instead of assigning it to an intermediate
indices variable.
In `@temoa/extensions/economies_of_scale/components/cost_invest_eos.py`:
- Around line 415-428: In the existing-capacity segment lookup around
cost_invest_eos_segment_cost, replace the truthiness-based `if not
prev_cum_cost` check with an explicit indicator that a segment matched. Set the
indicator when the capacity falls within a segment, so a legitimate zero cost is
accepted; raise the existing ValueError only when no segment matches.
In `@temoa/extensions/economies_of_scale/core/data_puller.py`:
- Around line 25-181: Extract the repeated exchange-versus-entries logic from
poll_costs into a shared _record_cost helper accepting the cost ledger, entries,
region, period, technology, both cost values, and their CostType values. Replace
all three investment, fixed, and variable branches with helper calls, preserving
the existing recording and accumulation behavior.
In `@temoa/extensions/framework.py`:
- Around line 166-183: Fix the interactive prompt construction in the
missing-table handling block so the input_database path is enclosed by a
properly closed quote before the “now? [y/N]:” text. Update only the prompt
assembled by the should_apply flow, preserving the existing confirmation
behavior.
- Around line 197-215: Update _table_has_rows to delegate table existence
validation to _table_exists instead of repeating the sqlite_master query; return
False when _table_exists reports the table is absent, then retain the existing
row query behavior for existing tables.
In `@temoa/extensions/growth_rates/data_manifest.py`:
- Around line 12-76: The build_manifest_items function duplicates six identical
LoadItem configurations; consolidate them into a loop over the distinct
growth/degrowth component and table pairs, constructing each LoadItem with the
shared columns, index_length, validator_name, validation_map, and flags.
Preserve the existing order and returned list contents.
In `@temoa/extensions/monte_carlo/mc_run.py`:
- Line 235: Update the cast call assigning self.settings_file to pass the actual
str type rather than the string literal "str", leaving the surrounding Path
conversion unchanged.
In `@temoa/extensions/template/components/example_limit.py`:
- Around line 18-19: Update the geography and technology imports in
example_limit.py to import each module directly without aliases that duplicate
their module names, and preserve the existing references to geography and
technology.
In `@temoa/extensions/template/core/model.py`:
- Around line 57-59: Update the domain of `m.example_new_capacity_limit` to
`NonNegativeReals` instead of `Any`, and replace the corresponding
`pyomo.environ` import so `NonNegativeReals` is available.
---
Outside diff comments:
In `@temoa/db_schema/temoa_schema_v4.sql`:
- Around line 919-938: Restore the foreign key constraint for output_cost.tech
in the output_cost table definition, referencing the Technology.tech key used by
Technology writes and downstream joins. Leave the existing columns and primary
key unchanged.
In `@temoa/extensions/get_comm_tech.py`:
- Around line 23-41: Wrap the SQLite operations in the connection-using block
around con, cur, and the scenario/period queries in a try/finally structure so
con.close() executes even when a query raises; keep cursor cleanup and existing
query behavior unchanged.
---
Duplicate comments:
In `@temoa/cli.py`:
- Around line 587-601: Extract the shared schema-first generation sequence from
the production and fallback paths into a
_build_tutorial_database(target_database, schema_sql, tutorial_sql) helper. Move
connection setup, schema/data execution, foreign-key toggling and validation,
violation handling, and commit into that helper, then have both paths call it
with their respective SQL sources while preserving the fallback path’s existing
from None behavior for IntegrityError.
In `@temoa/extensions/economies_of_scale/components/cost_variable_eos.py`:
- Around line 37-83: Update initialize_components so the first sorted segment in
model.cost_variable_eos_segments, identified by i == 0, validates that act_lower
and cost_lower are approximately zero before continuing. Preserve the existing
nonnegativity and monotonicity checks, and raise the same validation error path
when the first segment is unanchored.
🪄 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: 1c3ce7e6-e09b-47cb-9732-b2e67e88e632
⛔ Files ignored due to path filters (4)
docs/source/images/eos_cost_curve.pngis excluded by!**/*.pngdocs/source/images/eos_cost_curve.svgis excluded by!**/*.svgdocs/source/images/etl_cost_curve.pngis excluded by!**/*.pngdocs/source/images/etl_cost_curve.svgis excluded by!**/*.svg
📒 Files selected for processing (99)
docs/source/computational_implementation.rstdocs/source/extensions.rstdocs/source/extensions/discrete_capacity.rstdocs/source/extensions/eos.rstdocs/source/extensions/growth_rates.rstdocs/source/index.rstdocs/source/mathematical_formulation.rstdocs/source/param_desc_and_tables.rstpyproject.tomlscripts/generate_etl_figure.pytemoa/__init__.pytemoa/_internal/exchange_tech_cost_ledger.pytemoa/_internal/run_actions.pytemoa/_internal/table_data_puller.pytemoa/_internal/temoa_sequencer.pytemoa/cli.pytemoa/components/costs.pytemoa/components/limits.pytemoa/components/utils.pytemoa/core/config.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/data_io/loader_manifest.pytemoa/db_schema/temoa_schema_v4.sqltemoa/extensions/discrete_capacity/__init__.pytemoa/extensions/discrete_capacity/components/__init__.pytemoa/extensions/discrete_capacity/components/discrete_capacity.pytemoa/extensions/discrete_capacity/core/__init__.pytemoa/extensions/discrete_capacity/core/model.pytemoa/extensions/discrete_capacity/data_manifest.pytemoa/extensions/discrete_capacity/extension.pytemoa/extensions/discrete_capacity/tables.sqltemoa/extensions/economies_of_scale/__init__.pytemoa/extensions/economies_of_scale/components/__init__.pytemoa/extensions/economies_of_scale/components/cost_fixed_eos.pytemoa/extensions/economies_of_scale/components/cost_invest_eos.pytemoa/extensions/economies_of_scale/components/cost_variable_eos.pytemoa/extensions/economies_of_scale/core/__init__.pytemoa/extensions/economies_of_scale/core/data_puller.pytemoa/extensions/economies_of_scale/core/model.pytemoa/extensions/economies_of_scale/data_manifest.pytemoa/extensions/economies_of_scale/extension.pytemoa/extensions/economies_of_scale/tables.sqltemoa/extensions/framework.pytemoa/extensions/get_comm_tech.pytemoa/extensions/growth_rates/__init__.pytemoa/extensions/growth_rates/components/__init__.pytemoa/extensions/growth_rates/components/growth_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity.pytemoa/extensions/growth_rates/components/growth_new_capacity_delta.pytemoa/extensions/growth_rates/core/__init__.pytemoa/extensions/growth_rates/core/model.pytemoa/extensions/growth_rates/data_manifest.pytemoa/extensions/growth_rates/extension.pytemoa/extensions/growth_rates/tables.sqltemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/method_of_morris/morris_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/hull.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/example_builds/scenario_analyzer.pytemoa/extensions/monte_carlo/example_builds/scenario_maker.pytemoa/extensions/monte_carlo/mc_run.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/myopic/evolution_updater.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytemoa/extensions/template/__init__.pytemoa/extensions/template/components/__init__.pytemoa/extensions/template/components/example_limit.pytemoa/extensions/template/core/__init__.pytemoa/extensions/template/core/model.pytemoa/extensions/template/data_manifest.pytemoa/extensions/template/extension.pytemoa/extensions/template/tables.sqltemoa/tutorial_assets/config_sample.tomltemoa/tutorial_assets/utopia.sqltests/conftest.pytests/test_cli.pytests/test_extensions.pytests/test_full_runs.pytests/test_table_writer.pytests/testing_configs/config_myopic_capacities.tomltests/testing_data/mediumville_sets.jsontests/testing_data/myopic_capacities.sqltests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.jsontests/testing_data/utopia_v3.sql
💤 Files with no reviewable changes (4)
- temoa/extensions/modeling_to_generate_alternatives/manager_factory.py
- docs/source/param_desc_and_tables.rst
- docs/source/mathematical_formulation.rst
- temoa/extensions/stochastics/stochastic_config.py
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.
NOTE currently based on top of PR #345
Add
eosextension — piecewise-linear economies-of-scale cost curvesDocumentation
Economies of Scale (EOS).pdf
Summary
This PR introduces the Economies of Scale (EOS) extension, which replaces or supplements flat per-unit costs with piecewise-linear cost curves driven by deployment or usage quantities. Three cost types are supported:
cost_invest_eoscost_invest_eoscost_fixed_eoscost_fixed_eoscost_variable_eoscost_variable_eosThe extension is disabled by default and enabled per run with
extensions = ["eos"]in the config.Formulation
All three features share a common binary-integer piecewise-linear formulation. For each technology cluster
(r, t)(or(r, p, t)for period-specific curves):cost_invest_eosdiffers from the other two in that its curve is global across time: each period inherits all previously installed capacity. The period cost is the incremental cumulative cost — the curve evaluated at end-of-period cumulative capacity minus the curve evaluated at the previous period's cumulative capacity (or at pre-existing capacity in the first period). The incremental cost is then discounted and amortised using the sameloan_cost/loan_cost_survival_curvefunctions as the basecost_invest.cost_fixed_eosandcost_variable_eosare period-specific: the curve parameters can change from period to period. Their costs are discounted and amortised using the samefixed_or_variable_costfunction as the base model.Files changed
New source files:
temoa/extensions/economies_of_scale/components/cost_invest_eos.py— investment learning-curve componenttemoa/extensions/economies_of_scale/components/cost_variable_eos.py— variable O&M activity-curve componentModified source files:
temoa/extensions/economies_of_scale/components/cost_fixed_eos.py— docstrings updated to EOS terminologytemoa/extensions/economies_of_scale/core/model.py— type hints and component registration for all three features; newregister_early_eos_componentsfunction for early-stage set/param constructiontemoa/extensions/economies_of_scale/data_manifest.py—LoadItemforcost_invest_eostemoa/extensions/economies_of_scale/extension.py—owned_tablesandregional_group_tablesupdatedtemoa/extensions/economies_of_scale/tables.sql—cost_invest_eostable schema addedtemoa/core/model.py— callsregister_early_eos_componentsduring model constructionDocumentation:
docs/source/extensions/eos.rst— comprehensive new reference page covering all three features: concept, table schema, sets, variables, constraints (viaautofunction), and objective contributiondocs/source/extensions.rst— toctree updated (replacesetlentry witheos)docs/source/images/eos_cost_curve.png/.svg— new figure showing the piecewise-linear cost curve with labelled segment bounds, active-segment shading, and quantity/cost axis ticksscripts/generate_etl_figure.py— rewritten as the EOS figure generatorNotes
tech_or_group: all members share the same curve and their contributions to the driving quantity are summed.cost_investin the base tables alongside the piecewise curve.existing_capacity) is included in the cumulative total forcost_invest_eosbut does not incur an investment charge — it is subtracted out when computing the incremental period cost.cost_invest_eoscluster must share the samelifetime_process,loan_lifetime_process, andloan_ratein every period they appear together; the extension validates this at model build time.Summary by CodeRabbit
New Features
Updates
Documentation