Fixing materials flows in myopic mode (take 2)#279
Open
idelder wants to merge 20 commits intoTemoaProject:unstablefrom
Open
Fixing materials flows in myopic mode (take 2)#279idelder wants to merge 20 commits intoTemoaProject:unstablefrom
idelder wants to merge 20 commits intoTemoaProject:unstablefrom
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:
WalkthroughMakes FI commodity fields optional, adds lifetime-aware existing-capacity and end‑of‑life processing across load/build/write components, tightens near‑zero flow filtering, adds custom loaders and a new viable filter key Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as HybridLoader
participant Manifest as ComponentManifest
participant Network as CommodityNetworkManager
participant Builder as NetworkModelData
participant Puller as TableDataPuller
participant Writer as TableWriter
participant Capacity as CapacityComponent
Loader->>Manifest: register custom loaders (_load_construction_input, _load_emission_embodied)
Loader->>Network: send filtered viable sets (rtv, rtv_eol)
Network->>Builder: supply viable sets, tech_uncap
Builder->>Puller: emit lifetime-aware lookup data (eeol, exs_cap, silent_rptv)
Puller->>Writer: produce flow rows (FI i/o may be None)
Writer->>Capacity: provide flows for retirement/construction linking
Capacity->>Builder: record used_techs and apply retirement/survival logic
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)
📝 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/tutorial_assets/utopia.sql (1)
1193-1215:⚠️ Potential issue | 🟠 MajorSync
output_curtailment.seasonto referenceseason_label(season)instead oftime_period(period).The tutorial asset's
output_curtailmenttable points this column totime_period (period), but the canonical schema and other output tables in this file point it toseason_label (season). Since the table writer (temoa/_internal/table_writer.py) populates this column with season labels, the tutorial will surface FK violations on curtailment rows.Proposed fix
- season TEXT - REFERENCES time_period (period), + season TEXT + REFERENCES season_label (season),Also applies to: output_flow_out_summary (lines 1291–1305).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/tutorial_assets/utopia.sql` around lines 1193 - 1215, The foreign key for the season column in the output_curtailment table is incorrectly referencing time_period(period); update the season FK to reference season_label(season) instead so it matches how the table_writer populates season labels (change the REFERENCES on column season from time_period (period) to season_label (season)). Apply the same fix for the season FK in the output_flow_out_summary table so both table definitions reference season_label(season) consistently.
🤖 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/_internal/table_writer.py`:
- Line 624: The sentinel check uses equality instead of identity; update the
conditional that tests fi.i and fi.o to use identity checks (is None) rather
than == None to avoid invoking overloaded __eq__ and satisfy Ruff E711—i.e.,
change the conditional that currently reads a comparison to None to use "is
None" for both fi.i and fi.o in the relevant block where fi is inspected.
In `@temoa/components/capacity.py`:
- Around line 654-656: Rename the unused loop variable e to _e in the
comprehension over model.emission_end_of_life.sparse_iterkeys() to satisfy the
linter: update the for loop signature from "for r, e, t, v in
model.emission_end_of_life.sparse_iterkeys():" to use "_e" instead of "e" so the
body that checks "(r, t, v) in model.retirement_periods" and calls
"model.used_techs.add(t)" remains unchanged.
In `@temoa/components/technology.py`:
- Around line 442-460: Fix the typo in the error message inside
check_existing_capacity: update the string that currently reads "but it not in
process periods" to "but is not in process periods" in the message built for
existing capacity entries that extend into future periods (the message that
references variables cap, life and checks (r, t, v) not in model.process_periods
and v + life > model.time_optimize.first()) so the logger.error and raised
ValueError contain the corrected grammar.
In `@temoa/model_checking/commodity_network_manager.py`:
- Around line 137-142: The loop unpacks items from self.orig_data.silent_rptv
but the middle variable p is unused; rename it to _p in the for statement (for
r, _p, t, v in self.orig_data.silent_rptv) to indicate intentional discard and
silence the Ruff B007 warning, leaving the body that adds to
valid_elements['rtv'], ['rt'], ['t'], and ['v'] unchanged.
---
Outside diff comments:
In `@temoa/tutorial_assets/utopia.sql`:
- Around line 1193-1215: The foreign key for the season column in the
output_curtailment table is incorrectly referencing time_period(period); update
the season FK to reference season_label(season) instead so it matches how the
table_writer populates season labels (change the REFERENCES on column season
from time_period (period) to season_label (season)). Apply the same fix for the
season FK in the output_flow_out_summary table so both table definitions
reference season_label(season) consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d6e9d05-13e1-4ba9-bf01-9e1d22288986
📒 Files selected for processing (16)
temoa/_internal/table_data_puller.pytemoa/_internal/table_writer.pytemoa/components/capacity.pytemoa/components/technology.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/db_schema/temoa_schema_v4.sqltemoa/model_checking/commodity_network_manager.pytemoa/model_checking/network_model_data.pytemoa/tutorial_assets/utopia.sqltests/test_material_results.pytests/test_network_model_data.pytests/testing_data/materials.sqltests/testing_data/mediumville.sqltests/testing_data/mediumville_sets.json
💤 Files with no reviewable changes (1)
- tests/testing_data/mediumville.sql
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/model_checking/network_model_data.py (1)
474-522: 🧹 Nitpick | 🔵 TrivialExtract the shared p0-capacity predicate.
The
v + lifetime == periods[0]plus positive-existing-capacity rule now lives in both the EOL-output and EEOL branches. A small helper would make future myopic boundary tweaks less likely to drift.♻️ Suggested extraction
+ def has_positive_p0_capacity( + r: Region, tech: Technology, v: Vintage, lifetime: int + ) -> bool: + return ( + v + lifetime == periods[0] + and lookup_data['exs_cap'].get((r, tech, v), 0) > 0 + )- is_p0_eol = ( - (p == periods[0]) - and (v + lifetime == p) - and lookup_data['exs_cap'].get((r, tech, v), 0) > 0 - ) + is_p0_eol = (p == periods[0]) and has_positive_p0_capacity(r, tech, v, lifetime)- if v + lifetime == periods[0] and lookup_data['exs_cap'].get((r, tech, v), 0) > 0: + if has_positive_p0_capacity(r, tech, v, lifetime): res.silent_rptv.add((r, periods[0], tech, v))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/model_checking/network_model_data.py` around lines 474 - 522, Extract the repeated "p0 capacity" predicate into a small helper (e.g., is_p0_with_existing_capacity(region, tech, v, lifetime, periods, lookup_data)) and replace the duplicated checks in both the loop over lookup_data['eol'] (where you currently compute is_p0_eol) and the post-loop check over lookup_data['eeol'] with calls to that helper; the helper should return True when (v + lifetime == periods[0]) and lookup_data['exs_cap'].get((r, tech, v), 0) > 0 so both branches reuse the exact same logic and avoid drift.
🤖 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 620-628: The current "living" check (is_living) is too permissive
because model.process_periods can include vintages with non-positive
existing_capacity (e.g., negative carry-over with an Efficiency entry); update
the logic so process_periods only marks vintages with positive capacity or
tighten the local check here. Specifically, either (A) change
populate_core_dictionaries() to add (r,t,v) to model.process_periods only when
value(model.existing_capacity[r, t, v]) > 0, or (B) modify this file’s check to
require value(model.existing_capacity[r, t, v]) > 0 in addition to (r,t,v) in
model.process_periods (i.e., make is_living = (r, t, v) in model.process_periods
and value(model.existing_capacity[r, t, v]) > 0) so
invalid/zero/negative-capacity vintages no longer resurrect retirements.
In `@temoa/components/technology.py`:
- Around line 450-460: The check for non-positive existing capacity currently
logs a warning but continues processing, causing the same row to hit the
process_periods validation; change the branch handling cap <= 0 in the loop
(where cap, r, t, v are used and logger.warning is called) to stop further
processing for that entry—e.g., after logging the warning immediately
continue/skip to the next iteration so the subsequent lifetime/process_periods
checks (which reference model.lifetime_process and model.process_periods) are
not executed for non-positive capacities.
In `@tests/test_material_results.py`:
- Around line 36-38: When mode == 'myopic', ensure the test actually applies and
verifies overrides instead of silently skipping: assert that
config.myopic_inputs is not None (or explicitly initialize it) before assigning,
then set config.myopic_inputs['view_depth'] and ['step_size'] from request.param
and immediately assert that config.myopic_inputs['view_depth'] ==
request.param['view_depth'] and config.myopic_inputs['step_size'] ==
request.param['step_size'] so the test fails if overrides are not applied; refer
to the variables mode, config.myopic_inputs, and request.param to locate where
to add these checks.
---
Outside diff comments:
In `@temoa/model_checking/network_model_data.py`:
- Around line 474-522: Extract the repeated "p0 capacity" predicate into a small
helper (e.g., is_p0_with_existing_capacity(region, tech, v, lifetime, periods,
lookup_data)) and replace the duplicated checks in both the loop over
lookup_data['eol'] (where you currently compute is_p0_eol) and the post-loop
check over lookup_data['eeol'] with calls to that helper; the helper should
return True when (v + lifetime == periods[0]) and lookup_data['exs_cap'].get((r,
tech, v), 0) > 0 so both branches reuse the exact same logic and avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a0c8f12-cf9f-453e-b8b7-35fecc64e83c
📒 Files selected for processing (9)
temoa/_internal/table_data_puller.pytemoa/_internal/table_writer.pytemoa/components/capacity.pytemoa/components/flows.pytemoa/components/technology.pytemoa/model_checking/commodity_network_manager.pytemoa/model_checking/network_model_data.pytemoa/types/model_types.pytests/test_material_results.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
temoa/components/technology.py (1)
451-453:⚠️ Potential issue | 🟡 MinorFix message assembly bug and lingering grammar in
check_existing_capacity.At Line 451,
msgonly gets the f-string; the literal on Line 452 is a standalone no-op, so the warning text is truncated. Also, Line 459 still has “but it not in process periods”.🛠️ Proposed fix
if cap <= 0: - msg = f'Existing capacity {r, t, v} has non-positive capacity {cap}. ' - 'This entry will be ignored.' + msg = ( + f'Existing capacity {r, t, v} has non-positive capacity {cap}. ' + 'This entry will be ignored.' + ) logger.warning(msg) continue @@ f'Existing capacity {r, t, v} with lifetime {life} and capacity {cap} ' - 'should extend into future periods but it not in process periods. ' + 'should extend into future periods but is not in process periods. ' 'Was it included in the Efficiency table?' )#!/bin/bash # Read-only verification of the flagged message lines sed -n '446,462p' temoa/components/technology.pyAlso applies to: 459-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/components/technology.py` around lines 451 - 453, The warning message assembly in check_existing_capacity is broken because the second literal is a standalone no-op; fix by combining into one string (e.g., build msg as a single f-string that includes both the existing capacity tuple and the note "This entry will be ignored.") and call logger.warning(msg). Also locate the later grammar bug in check_existing_capacity where the message reads "but it not in process periods" and correct it to "but it is not in process periods." Ensure you reference the same variable names (msg, cap) and the function check_existing_capacity when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_material_results.py`:
- Around line 140-143: Update the SQL string in the cursor.execute call (the
SELECT SUM(flow) query assigned to variable row in test_material_results.py) to
use consistent keyword casing by replacing the lowercase "and" with "AND" so all
SQL keywords (WHERE, AND) are uppercase for readability.
---
Duplicate comments:
In `@temoa/components/technology.py`:
- Around line 451-453: The warning message assembly in check_existing_capacity
is broken because the second literal is a standalone no-op; fix by combining
into one string (e.g., build msg as a single f-string that includes both the
existing capacity tuple and the note "This entry will be ignored.") and call
logger.warning(msg). Also locate the later grammar bug in
check_existing_capacity where the message reads "but it not in process periods"
and correct it to "but it is not in process periods." Ensure you reference the
same variable names (msg, cap) and the function check_existing_capacity when
making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 808349dc-1d65-497a-9a1f-ce2093c2a44c
📒 Files selected for processing (2)
temoa/components/technology.pytests/test_material_results.py
idelder
force-pushed
the
bugfix/redo/materials_myopic
branch
from
f197b33 to
2e863e6
Compare
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 `@temoa/_internal/table_data_puller.py`:
- Around line 110-111: The current clamping (early = 0 if early < epsilon else
early; eol = 0 if eol < epsilon else eol) zeroes any negative value; change it
so only near-zero noise is cleared and materially negative values are surfaced:
replace the condition with a near-zero check (e.g., early = 0 if abs(early) <
epsilon else early and likewise for eol) and add handling for significant
negatives (e.g., log.warning or raise ValueError when early < -epsilon or eol <
-epsilon) inside the same scope in table_data_puller (referencing the early,
eol, and epsilon symbols) so large negative retirement components are not
silently masked.
In `@temoa/components/technology.py`:
- Around line 163-169: The code currently logs an error then raises ValueError
when a process has non-positive existing capacity while t not in
model.tech_uncap, which is inconsistent with check_existing_capacity (which only
warns and skips such rows); change the hard failure to mirror that behavior:
replace the logger.error + raise in the block that checks model.tech_uncap and
value(model.existing_capacity[process]) with a logger.warning (or info) that
includes the process and capacity details and then skip further handling of this
process for the current context (i.e., continue/return from the enclosing loop
or function as appropriate) so non-positive existing_capacity rows are
warned-and-ignored rather than aborting execution, consistent with
check_existing_capacity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0bd5ed2-108b-4ed6-9187-0191a322827b
📒 Files selected for processing (3)
temoa/_internal/table_data_puller.pytemoa/components/technology.pytests/test_material_results.py
…ing exactly on p0
…l to avoid foreign key issues
idelder
force-pushed
the
bugfix/redo/materials_myopic
branch
from
87d77fa to
0c5533d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/_internal/table_writer.py (1)
545-571: 🧹 Nitpick | 🔵 TrivialLGTM! Robust handling of optional commodity identifiers.
The updated signature correctly handles
Nonevalues forinput_commandoutput_comm, which aligns with the new FI representations for construction inputs and end-of-life outputs. The guard conditions properly ensure unit resolution only proceeds when the relevant commodity is available.Minor style note: The static analyzer flags line 568's
elifas unnecessary after thereturnstatement (RET505). This is a minor readability preference and doesn't affect correctness.Optional style refinement
if flow_type == FlowType.IN and input_comm is not None: return unit_prop.get_flow_in_units(input_comm) - elif output_comm is not None: + if output_comm is not None: # OUT, CURTAIL, FLEX all use output commodity units return unit_prop.get_flow_out_units(output_comm) return None,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/_internal/table_writer.py` around lines 545 - 571, The static analyzer flags the unnecessary "elif" after a prior return in _get_flow_units; change the "elif output_comm is not None:" branch in the _get_flow_units method to a plain "if output_comm is not None:" (or otherwise restructure the control flow) so there is no unreachable conditional following a return while keeping the same logic that OUT/CURTAIL/FLEX use unit_propagator.get_flow_out_units(output_comm).
🤖 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/db_schema/temoa_schema_v4.sql`:
- Around line 1091-1106: The PRIMARY KEY columns for table
output_flow_out_summary (scenario, region, period, input_comm, tech, vintage,
output_comm) are not marked NOT NULL, allowing NULLs to bypass uniqueness in
SQLite; update the table definition in temoa_schema_v4.sql (and the mirrored
utopia.sql) to add NOT NULL to each of those columns (scenario, region, sector
if part of key, period, input_comm, tech, vintage, output_comm) so the composite
key enforces uniqueness, or alternatively replace the composite PK with a
surrogate id column and add an explicit UNIQUE constraint on the intended
composite set if you prefer a surrogate-key approach.
In `@temoa/tutorial_assets/utopia.sql`:
- Around line 1138-1145: The tutorial schema's output_curtailment.season FK
currently references time_period but must mirror the main schema: change the
FOREIGN KEY on the output_curtailment.season column to reference season_label
instead of time_period so inserts that use the season label (fi.s written by
table_writer.py lines handling curtailment rows) won't be rejected; locate the
output_curtailment table definition and update the season column's REFERENCES
clause from time_period (period) to season_label (season_label).
In `@tests/test_material_results.py`:
- Around line 61-123: Add a new regression case to the flow_tests list in
tests/test_material_results.py that exercises the silent end-of-life emissions
branch: append a dict (e.g. name 'lithium import - emission-only EOL', tech
'IMPORT_LI', period 1990, mode 'perfect_foresight', target same as others) so
the solver encounters a retiree with only emission_end_of_life and no
end_of_life_output; then update the test's post-solve assertions (the block that
iterates flow_tests) to assert the solve succeeds and that the
emission_end_of_life output row was written (check for presence/expected value
in emission_end_of_life) without requiring any matching end_of_life_output
entry. Ensure you reference the flow_tests variable and the output table names
'emission_end_of_life' and 'end_of_life_output' when adding the case and
assertions.
---
Outside diff comments:
In `@temoa/_internal/table_writer.py`:
- Around line 545-571: The static analyzer flags the unnecessary "elif" after a
prior return in _get_flow_units; change the "elif output_comm is not None:"
branch in the _get_flow_units method to a plain "if output_comm is not None:"
(or otherwise restructure the control flow) so there is no unreachable
conditional following a return while keeping the same logic that
OUT/CURTAIL/FLEX use unit_propagator.get_flow_out_units(output_comm).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c471490-ef2e-41cd-8598-fcd3c3812020
📒 Files selected for processing (18)
temoa/_internal/table_data_puller.pytemoa/_internal/table_writer.pytemoa/components/capacity.pytemoa/components/flows.pytemoa/components/technology.pytemoa/core/model.pytemoa/data_io/component_manifest.pytemoa/data_io/hybrid_loader.pytemoa/db_schema/temoa_schema_v4.sqltemoa/model_checking/commodity_network_manager.pytemoa/model_checking/network_model_data.pytemoa/tutorial_assets/utopia.sqltemoa/types/model_types.pytests/test_material_results.pytests/test_network_model_data.pytests/testing_data/materials.sqltests/testing_data/mediumville.sqltests/testing_data/mediumville_sets.json
💤 Files with no reviewable changes (1)
- tests/testing_data/mediumville.sql
Comment on lines
+1091
to
1106
| scenario TEXT, | ||
| region TEXT, | ||
| sector TEXT | ||
| REFERENCES sector_label (sector), | ||
| period INTEGER | ||
| REFERENCES time_period (period), | ||
| input_comm TEXT | ||
| REFERENCES commodity (name), | ||
| tech TEXT | ||
| REFERENCES technology (tech), | ||
| vintage INTEGER | ||
| REFERENCES time_period (period), | ||
| output_comm TEXT | ||
| REFERENCES commodity (name), | ||
| flow REAL, | ||
| PRIMARY KEY (scenario, region, period, input_comm, tech, vintage, output_comm) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import sqlite3
con = sqlite3.connect(":memory:")
cur = con.cursor()
cur.execute("""
CREATE TABLE t (
scenario TEXT,
region TEXT,
period INTEGER,
input_comm TEXT,
tech TEXT,
vintage INTEGER,
output_comm TEXT,
PRIMARY KEY (scenario, region, period, input_comm, tech, vintage, output_comm)
)
""")
row = (None, "utopia", 2000, "ethos", "E31", 2000, "ELC")
cur.execute("INSERT INTO t VALUES (?, ?, ?, ?, ?, ?, ?)", row)
cur.execute("INSERT INTO t VALUES (?, ?, ?, ?, ?, ?, ?)", row)
print("rows:", cur.execute("SELECT COUNT(*) FROM t").fetchone()[0])
print("table_info:", cur.execute("PRAGMA table_info(t)").fetchall())
PYRepository: TemoaProject/temoa
Length of output: 343
🏁 Script executed:
# Check the actual definition of output_flow_out_summary in the schema file
rg -A 20 "CREATE TABLE.*output_flow_out_summary" temoa/db_schema/ --type sqlRepository: TemoaProject/temoa
Length of output: 1399
🏁 Script executed:
# Check if there's a mirrored definition in tutorial assets
rg -A 20 "CREATE TABLE.*output_flow_out_summary" temoa/tutorial_assets/ --type sqlRepository: TemoaProject/temoa
Length of output: 1348
🏁 Script executed:
# Check for any comments or documentation about this table's nullable key behavior
rg "output_flow_out_summary" temoa/ -B 2 -A 2 --type sql --type md --type txtRepository: TemoaProject/temoa
Length of output: 1394
Add NOT NULL constraints to composite key columns in output_flow_out_summary.
The composite PRIMARY KEY (scenario, region, period, input_comm, tech, vintage, output_comm) lacks NOT NULL constraints on its columns. In SQLite, NULL values bypass uniqueness enforcement—duplicate rows with NULL in any key column are allowed. Add NOT NULL to all key columns, or switch to a surrogate key with explicit uniqueness constraints.
This issue exists in both temoa/db_schema/temoa_schema_v4.sql and the mirrored definition in temoa/tutorial_assets/utopia.sql.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/db_schema/temoa_schema_v4.sql` around lines 1091 - 1106, The PRIMARY
KEY columns for table output_flow_out_summary (scenario, region, period,
input_comm, tech, vintage, output_comm) are not marked NOT NULL, allowing NULLs
to bypass uniqueness in SQLite; update the table definition in
temoa_schema_v4.sql (and the mirrored utopia.sql) to add NOT NULL to each of
those columns (scenario, region, sector if part of key, period, input_comm,
tech, vintage, output_comm) so the composite key enforces uniqueness, or
alternatively replace the composite PK with a surrogate id column and add an
explicit UNIQUE constraint on the intended composite set if you prefer a
surrogate-key approach.
Comment on lines
+1138
to
+1145
| input_comm TEXT | ||
| REFERENCES commodity (name), | ||
| tech TEXT | ||
| REFERENCES technology (tech), | ||
| vintage INTEGER | ||
| REFERENCES time_period (period), | ||
| output_comm TEXT | ||
| REFERENCES commodity (name), |
There was a problem hiding this comment.
Mirror the output_curtailment.season FK fix into the tutorial schema.
temoa/db_schema/temoa_schema_v4.sql now points output_curtailment.season at season_label, but this asset still uses time_period on Line 1201. Cross-file, temoa/_internal/table_writer.py:500-541 writes the season label (fi.s) into that column for curtailment rows, so FK enforcement can still reject valid inserts here.
🛠️ Minimal fix
- season TEXT
- REFERENCES time_period (period),
+ season TEXT
+ REFERENCES season_label (season),Also applies to: 1291-1305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/tutorial_assets/utopia.sql` around lines 1138 - 1145, The tutorial
schema's output_curtailment.season FK currently references time_period but must
mirror the main schema: change the FOREIGN KEY on the output_curtailment.season
column to reference season_label instead of time_period so inserts that use the
season label (fi.s written by table_writer.py lines handling curtailment rows)
won't be rejected; locate the output_curtailment table definition and update the
season column's REFERENCES clause from time_period (period) to season_label
(season_label).
Comment on lines
61
to
123
| flow_tests = [ | ||
| {'name': 'lithium import', 'tech': 'IMPORT_LI', 'period': 2000, 'target': 0.129291623}, | ||
| { | ||
| 'name': 'lithium import - perfect foresight', | ||
| 'tech': 'IMPORT_LI', | ||
| 'period': 2000, | ||
| 'target': 0.129291623, | ||
| 'mode': 'perfect_foresight', | ||
| }, | ||
| { | ||
| 'name': 'lithium import - myopic 1-1', | ||
| 'tech': 'IMPORT_LI', | ||
| 'period': 2000, | ||
| 'target': 0.129291623, | ||
| 'mode': 'myopic', | ||
| 'view_depth': 1, | ||
| 'step_size': 1, | ||
| }, | ||
| { | ||
| 'name': 'lithium import - myopic 2-1', | ||
| 'tech': 'IMPORT_LI', | ||
| 'period': 2000, | ||
| 'target': 0.129291623, | ||
| 'mode': 'myopic', | ||
| 'view_depth': 2, | ||
| 'step_size': 1, | ||
| }, | ||
| { | ||
| 'name': 'lithium import - myopic 3-1', | ||
| 'tech': 'IMPORT_LI', | ||
| 'period': 2000, | ||
| 'target': 0.129291623, | ||
| 'mode': 'myopic', | ||
| 'view_depth': 3, | ||
| 'step_size': 1, | ||
| }, | ||
| { | ||
| 'name': 'lithium import - myopic 2-2', | ||
| 'tech': 'IMPORT_LI', | ||
| 'period': 2000, | ||
| 'target': 0.129291623, | ||
| 'mode': 'myopic', | ||
| 'view_depth': 2, | ||
| 'step_size': 2, | ||
| }, | ||
| { | ||
| 'name': 'lithium import - myopic 3-2', | ||
| 'tech': 'IMPORT_LI', | ||
| 'period': 2000, | ||
| 'target': 0.129291623, | ||
| 'mode': 'myopic', | ||
| 'view_depth': 3, | ||
| 'step_size': 2, | ||
| }, | ||
| { | ||
| 'name': 'lithium import - myopic 3-3', | ||
| 'tech': 'IMPORT_LI', | ||
| 'period': 2000, | ||
| 'target': 0.129291623, | ||
| 'mode': 'myopic', | ||
| 'view_depth': 3, | ||
| 'step_size': 3, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add one emission-only v + lifetime == p0 regression case.
This matrix covers the myopic parameter combinations, but it still never exercises the new silent EOL-emissions branch from this PR: tests/testing_data/materials.sql has no emission_end_of_life rows, and every 1990 retiree also has end_of_life_output. Add one case with only emission_end_of_life, then assert the solve succeeds and the expected emission output is written without requiring a matching end_of_life_output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_material_results.py` around lines 61 - 123, Add a new regression
case to the flow_tests list in tests/test_material_results.py that exercises the
silent end-of-life emissions branch: append a dict (e.g. name 'lithium import -
emission-only EOL', tech 'IMPORT_LI', period 1990, mode 'perfect_foresight',
target same as others) so the solver encounters a retiree with only
emission_end_of_life and no end_of_life_output; then update the test's
post-solve assertions (the block that iterates flow_tests) to assert the solve
succeeds and that the emission_end_of_life output row was written (check for
presence/expected value in emission_end_of_life) without requiring any matching
end_of_life_output entry. Ensure you reference the flow_tests variable and the
output table names 'emission_end_of_life' and 'end_of_life_output' when adding
the case and assertions.
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
2 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.
Materials flows were not compatible with myopic mode. This is because a process can reach end of life exactly on the start of the planning horizon (vintage + lifetime == p0) so the process was filtered out by network checks, the model didn't know it existed, but it was still trying to load in these end of life flows. This makes some fixes for that:
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests