Closed
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:
WalkthroughReplace commodity-string markers for construction/end-of-life flows with None, add an end-of-life viability filter and myopic custom loaders, change small-value pruning to sign-aware checks, aggregate retirements and active-capacity indexing by (region,technology,vintage), extend network EOL/construction edges with lifetime-aware logic, update DB FKs and nullability, and extend tests to run two scenario modes. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as HybridLoader
participant Model as TemoaModel
participant Network as CommodityNetworkManager
participant DB as Database
participant Validator as check_existing_capacity
Loader->>Model: _load_construction_input / _load_emission_embodied (apply myopic filter)
Loader->>Model: populate model.construction_input / model.emission_embodied
Model->>Validator: run check_existing_capacity(model)
Validator-->>Model: pass / raise
Model->>Network: build_filters() (includes rtv_eol)
Network->>DB: query lookup rows (periods, eol with lifetime)
DB-->>Network: return lookup rows
Network->>Model: emit EdgeTuples (construction/EOL edges with actual tech + lifetime)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Author
|
Waiting for Dijuan to stress test this a bit before clearing. Very finicky logic. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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`:
- Line 193: The code constructs FI(...) with a None commodity sentinel but
TableWriter._insert_summary_flow_results writes fi.i and fi.o directly into
output_flow_out_summary whose input_comm/output_comm columns are declared NOT
NULL, causing end-of-life summary inserts to fail; either filter out FI
instances with a None commodity before calling _insert_summary_flow_results (or
skip writing rows where fi.i or fi.o is None) or change the summary
schema/column definitions to accept NULLs and propagate nullable types through
the writer; locate the FI construction site and the
TableWriter._insert_summary_flow_results method and implement one of these fixes
so fi.i/fi.o are never passed as non-nullable values to output_flow_out_summary.
In `@temoa/components/capacity.py`:
- Around line 607-610: Replace the unnecessary set(...) wrapping a generator
when building unique_rtv with a set comprehension; specifically, change the
expression that creates unique_rtv (which currently uses set((r, t, v) for r,
_i, t, v, _o in model.efficiency.sparse_iterkeys()) |
set(model.existing_capacity.sparse_iterkeys())) to use a single set
comprehension for the first part (e.g., {(r, t, v) for r, _i, t, v, _o in
model.efficiency.sparse_iterkeys()}) and keep the union with
set(model.existing_capacity.sparse_iterkeys()) so unique_rtv is created
idiomatically and without the unnecessary generator.
In `@temoa/components/technology.py`:
- Around line 441-464: The check in check_existing_capacity wrongly rejects
retirement-only existing_capacity tuples because it requires membership in
model.active_capacity_available_rptv (derived from process_vintages/efficiency);
change the validation so that for each (r,t,v) you first skip the
active_capacity check when the vintage is only present for retirement/EOL
bookkeeping (i.e., not part of the process network), or alternatively validate
against the retirement/EOL bookkeeping sets (the structures populated by
populate_core_dictionaries for retirement entries) instead of
active_capacity_available_rptv; update the loop that inspects
model.time_optimize and the membership test (r, p, t, v) not in
model.active_capacity_available_rptv to only apply when (r,t,v) participates in
the process network (or to check the retirement bookkeeping set) while keeping
the existing tech_uncap and lifetime_process checks.
In `@temoa/db_schema/temoa_schema_v4.sql`:
- Line 711: The schema change to output_curtailment (altered FK to
season_label(season)) requires updating the schema version or adding a
migration: either increment DB_MINOR in temoa_schema_v4.sql and update any
version constants, or add an explicit migration that alters the existing output
DBs to drop the old FK and recreate the new FK; ensure
TableWriter.clear_scenario() behavior is considered so existing output DBs are
migrated (or recreated) at runtime, and reference the output_curtailment table
name, the DB_MINOR constant, and the TableWriter.clear_scenario() call site when
implementing the change.
In `@temoa/model_checking/commodity_network_manager.py`:
- Around line 129-131: rtv_eol is being populated with input commodities instead
of technologies; change the additions to use edge_tuple.tech (not
edge_tuple.input_comm) and only add tuples for edges that are actual
retirement/EOL edges (i.e., where the edge represents an end-of-life/retirement
output), so replace the current unconditional adds to valid_elements['rtv_eol']
with logic that: checks the edge is a retirement/EOL edge and then adds
(edge_tuple.region, edge_tuple.tech, edge_tuple.vintage). Apply the same fix to
the other occurrence referenced around lines 163-167.
In `@tests/test_material_results.py`:
- Line 89: The unpacked variable `mode` from the `solved_connection` fixture is
unused; change the unpacking in the test from "con, name, tech, period,
flow_target, mode = solved_connection" to prefix the unused variable with an
underscore (e.g., "con, name, tech, period, flow_target, _mode =
solved_connection") so linters (Ruff RUF059) know it's intentionally unused;
keep all other uses of the other variables unchanged and only rename the
unpacked symbol in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5de40ea2-4aeb-4a2b-a662-d438a2817791
📒 Files selected for processing (14)
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.pytests/test_material_results.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
idelder
commented
Mar 14, 2026
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/model_checking/network_model_data.py (1)
359-443:⚠️ Potential issue | 🟠 MajorRestrict survival-curve EOL edges to periods where the vintage can retire.
Line 359 no longer gates the rest of the loop, so the EOL branch now runs for every period. With
has_survivaldefined only by tuple membership, survival-curve techs get EOL edges beforevand afterv + lifetime, which pollutes the network data. Bound that branch byv <= p <= v + lifetime.♻️ Suggested fix
- has_survival = (r, tech, v) in basic_data['tech_survival_curve'] + has_survival = ( + (r, tech, v) in basic_data['tech_survival_curve'] + and v <= p <= v + lifetime + )🤖 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 359 - 443, The survival-curve EOL branch is executing for periods outside the technology's active vintage because has_survival is only checked by tuple membership; restrict that branch to periods where the vintage is active by adding a condition v <= p <= v + lifetime (so the EOL loop runs only when the vintage could be alive). Update the conditional that currently checks is_natural_eol or is_retireable or has_survival (around the has_survival computation and the lookup_data['eol'] loop) to include the v <= p <= v + lifetime guard before adding entries to res.available_techs, res.source_commodities and res.capacity_commodities and before adding to living_techs.
🤖 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/technology.py`:
- Around line 441-470: In check_existing_capacity, after the tech_uncap guard
(inside the loop over model.existing_capacity.sparse_iterkeys), skip rows whose
existing capacity is explicitly zero instead of erroring; retrieve the stored
capacity for (r, t, v) from model.existing_capacity (or via
value(model.existing_capacity[r, t, v])) and if it equals 0, continue the loop
before computing lifetime and checking active_capacity_available_rptv so
explicit zero-capacity rows don't cause a ValueError.
---
Outside diff comments:
In `@temoa/model_checking/network_model_data.py`:
- Around line 359-443: The survival-curve EOL branch is executing for periods
outside the technology's active vintage because has_survival is only checked by
tuple membership; restrict that branch to periods where the vintage is active by
adding a condition v <= p <= v + lifetime (so the EOL loop runs only when the
vintage could be alive). Update the conditional that currently checks
is_natural_eol or is_retireable or has_survival (around the has_survival
computation and the lookup_data['eol'] loop) to include the v <= p <= v +
lifetime guard before adding entries to res.available_techs,
res.source_commodities and res.capacity_commodities and before adding to
living_techs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f1be936-055d-424e-b67f-9c27546b50ba
📒 Files selected for processing (4)
temoa/components/capacity.pytemoa/components/technology.pytemoa/model_checking/network_model_data.pytests/test_material_results.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
temoa/tutorial_assets/utopia.sql (3)
1134-1151: 🧹 Nitpick | 🔵 TrivialRemove redundant FK constraint (same as schema).
Line 1149 declares
FOREIGN KEY (tech) REFERENCES technology (tech), but lines 1140-1141 already define this inline.🧹 Proposed fix
efficiency real, lifetime integer, - FOREIGN KEY (tech) REFERENCES technology (tech), PRIMARY KEY (region, input_comm, tech, vintage, output_comm) );🤖 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 1134 - 1151, The CREATE TABLE myopic_efficiency has a redundant foreign key constraint: remove the duplicate table-level clause "FOREIGN KEY (tech) REFERENCES technology (tech)" because the column "tech TEXT REFERENCES technology (tech)" already defines the same FK; update the myopic_efficiency definition to keep the inline REFERENCES on the tech column and delete the separate FOREIGN KEY (tech) ... line.
1195-1217:⚠️ Potential issue | 🟠 MajorMissing update:
output_curtailment.seasonFK inconsistent with schema.The schema file (
temoa_schema_v4.sqlline 711) changedoutput_curtailment.seasonto referenceseason_label(season), but this file still referencestime_period(period)at line 1203.This FK is semantically incorrect — season values in this database are labels like
'inter','summer','winter'(seeseason_labelinserts at lines 1435-1437), not period integers.🐛 Proposed fix
period INTEGER REFERENCES time_period (period), season TEXT - REFERENCES time_period (period), + REFERENCES season_label (season), tod TEXT🤖 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 1195 - 1217, The output_curtailment table's season column currently references time_period(period) but should reference season_label(season); update the CREATE TABLE for output_curtailment so the season column's FOREIGN KEY references season_label(season) (retain the column type TEXT and other constraints), ensuring the PRIMARY KEY and other references remain unchanged; target the output_curtailment definition and change the REFERENCES clause for the season column to season_label(season).
1291-1311: 🧹 Nitpick | 🔵 TrivialChanges align with schema; remove redundant FK.
FK additions and nullability changes correctly mirror
temoa_schema_v4.sql. Line 1309 duplicates the inline FK at lines 1301-1302.🧹 Remove duplicate FK
flow REAL, - FOREIGN KEY (tech) REFERENCES technology (tech), PRIMARY KEY (scenario, region, period, input_comm, tech, vintage, output_comm) );🤖 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 1291 - 1311, The CREATE TABLE output_flow_out_summary contains a duplicated foreign key for technology: remove the redundant FOREIGN KEY (tech) REFERENCES technology (tech) clause (the tech column already has an inline REFERENCES technology (tech)), keeping the tech column definition and the PRIMARY KEY intact; verify output_flow_out_summary still matches temoa_schema_v4.sql after removing that duplicate FK.temoa/db_schema/temoa_schema_v4.sql (2)
1091-1111: 🧹 Nitpick | 🔵 TrivialNullability changes support materials module; remove redundant FK.
The removal of NOT NULL constraints for
scenario,region,input_comm, andflowcorrectly enables writing partial records for construction/EOL material flows per PR objectives.However, line 1109 duplicates the FK constraint already declared inline at lines 1101-1102.
🧹 Remove duplicate FK
flow REAL, - FOREIGN KEY (tech) REFERENCES technology (tech), PRIMARY KEY (scenario, region, period, input_comm, tech, vintage, output_comm) );🤖 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 - 1111, The table output_flow_out_summary declares tech TEXT REFERENCES technology (tech) inline, but also repeats the same FOREIGN KEY (tech) REFERENCES technology (tech) in the table constraints; remove the duplicate table-level FOREIGN KEY constraint so only the inline REFERENCES on the tech column remains (search for output_flow_out_summary and the redundant FOREIGN KEY (tech) REFERENCES technology (tech) and delete that table-constraint entry).
1074-1086: 🧹 Nitpick | 🔵 TrivialRemove redundant FK constraint.
Line 1085 declares
FOREIGN KEY (tech) REFERENCES technology (tech), but this constraint is already defined inline at lines 1076-1077. The duplicate is harmless but adds clutter.🧹 Proposed fix
efficiency real, lifetime integer, - FOREIGN KEY (tech) REFERENCES technology (tech), PRIMARY KEY (region, input_comm, tech, vintage, output_comm) );🤖 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 1074 - 1086, Remove the redundant foreign-key constraint that repeats the inline reference on the tech column: there is already an inline "tech TEXT REFERENCES technology (tech)" definition, so delete the separate "FOREIGN KEY (tech) REFERENCES technology (tech)" constraint to avoid duplication and clutter while keeping the inline reference and the existing PRIMARY KEY (region, input_comm, tech, vintage, output_comm) intact.
🤖 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 36-38: The test assigns into config.myopic_inputs unconditionally;
change it so these assignments only run when mode == "myopic" and, if mode ==
"myopic" but config.myopic_inputs is None, fail fast with a clear test error
(e.g., raise AssertionError or pytest.fail) explaining that MYOPIC mode requires
myopic_inputs; reference config.myopic_inputs and the test's mode variable and
ensure this check is added before the assignments (this mirrors the expectations
in HybridLoader.create_data_dict).
---
Outside diff comments:
In `@temoa/db_schema/temoa_schema_v4.sql`:
- Around line 1091-1111: The table output_flow_out_summary declares tech TEXT
REFERENCES technology (tech) inline, but also repeats the same FOREIGN KEY
(tech) REFERENCES technology (tech) in the table constraints; remove the
duplicate table-level FOREIGN KEY constraint so only the inline REFERENCES on
the tech column remains (search for output_flow_out_summary and the redundant
FOREIGN KEY (tech) REFERENCES technology (tech) and delete that table-constraint
entry).
- Around line 1074-1086: Remove the redundant foreign-key constraint that
repeats the inline reference on the tech column: there is already an inline
"tech TEXT REFERENCES technology (tech)" definition, so delete the separate
"FOREIGN KEY (tech) REFERENCES technology (tech)" constraint to avoid
duplication and clutter while keeping the inline reference and the existing
PRIMARY KEY (region, input_comm, tech, vintage, output_comm) intact.
In `@temoa/tutorial_assets/utopia.sql`:
- Around line 1134-1151: The CREATE TABLE myopic_efficiency has a redundant
foreign key constraint: remove the duplicate table-level clause "FOREIGN KEY
(tech) REFERENCES technology (tech)" because the column "tech TEXT REFERENCES
technology (tech)" already defines the same FK; update the myopic_efficiency
definition to keep the inline REFERENCES on the tech column and delete the
separate FOREIGN KEY (tech) ... line.
- Around line 1195-1217: The output_curtailment table's season column currently
references time_period(period) but should reference season_label(season); update
the CREATE TABLE for output_curtailment so the season column's FOREIGN KEY
references season_label(season) (retain the column type TEXT and other
constraints), ensuring the PRIMARY KEY and other references remain unchanged;
target the output_curtailment definition and change the REFERENCES clause for
the season column to season_label(season).
- Around line 1291-1311: The CREATE TABLE output_flow_out_summary contains a
duplicated foreign key for technology: remove the redundant FOREIGN KEY (tech)
REFERENCES technology (tech) clause (the tech column already has an inline
REFERENCES technology (tech)), keeping the tech column definition and the
PRIMARY KEY intact; verify output_flow_out_summary still matches
temoa_schema_v4.sql after removing that duplicate FK.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ef5b092-3148-4d64-9015-a6ed3a285987
📒 Files selected for processing (3)
temoa/db_schema/temoa_schema_v4.sqltemoa/tutorial_assets/utopia.sqltests/test_material_results.py
… unstable models and mess up myopic mode
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/model_checking/network_model_data.py`:
- Around line 294-306: Remove the unnecessary f-string prefix from the query
(use a normal triple-quoted string for variable query) and rename the ambiguous
loop variable l to lifetime for clarity; update the for-loop unpacking to "for
r, tech, v, oc, lifetime in cur.execute(query,
(TemoaModel.default_lifetime_tech,)).fetchall():" and adjust the lookups access
to use lifetime (lookups['eol'][(r, tech, v, lifetime)].append(oc)), leaving
TemoaModel.default_lifetime_tech, query, cur.execute, and lookups['eol']
references unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e97fc11f-6959-4874-8931-7eb36b0f751c
📒 Files selected for processing (5)
temoa/_internal/table_data_puller.pytemoa/components/capacity.pytemoa/db_schema/temoa_schema_v4.sqltemoa/model_checking/network_model_data.pytemoa/tutorial_assets/utopia.sql
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)
tests/test_network_model_data.py (1)
279-286: 🧹 Nitpick | 🔵 TrivialConsider combining adjacent branches returning identical results.
Both the
unlim_cap==1andretire==1branches return empty results. Combining them would reduce duplication.♻️ Optional refactor
- elif 'technology WHERE unlim_cap==1' in query: - m = MagicMock() - m.fetchall.return_value = [] - return m - elif 'technology WHERE retire==1' in query: + elif 'technology WHERE unlim_cap==1' in query or 'technology WHERE retire==1' in query: m = MagicMock() m.fetchall.return_value = [] return mThat said, keeping them separate is also reasonable for maintainability if these queries may need different mock data in future tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_network_model_data.py` around lines 279 - 286, The two adjacent branches checking for 'technology WHERE unlim_cap==1' and 'technology WHERE retire==1' both create a MagicMock with m.fetchall.return_value = [] and return it; replace them with a single conditional that matches either substring (e.g., using an or or membership check) and returns the single MagicMock to remove duplication while preserving behavior in the test helper that inspects the query string.
♻️ Duplicate comments (1)
tests/test_network_model_data.py (1)
367-374: 🧹 Nitpick | 🔵 TrivialSame optional refactor opportunity as above.
Consider combining with the adjacent
retire==1branch for consistency.♻️ Optional refactor
- elif 'technology WHERE unlim_cap==1' in query: - m = MagicMock() - m.fetchall.return_value = [] - return m - elif 'technology WHERE retire==1' in query: + elif 'technology WHERE unlim_cap==1' in query or 'technology WHERE retire==1' in query: m = MagicMock() m.fetchall.return_value = [] return m🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_network_model_data.py` around lines 367 - 374, The two separate elif branches checking for 'technology WHERE unlim_cap==1' and 'technology WHERE retire==1' both create the same MagicMock with an empty fetchall; replace them with a single branch that detects either query (e.g., combine the two 'in' checks with an or) and returns one MagicMock with m.fetchall.return_value = [] to avoid duplicated code; locate the existing elifs that reference the exact strings 'technology WHERE unlim_cap==1' and 'technology WHERE retire==1' and merge their logic accordingly.
🤖 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 644-648: The loop currently calls model.used_techs.add(t) for
every end_of_life_output row even when no retirement flows are created; restrict
marking techs as used to only those that actually produce a retirement flow by
moving the model.used_techs.add(t) call inside the if (r, t, v) in
model.retirement_periods block (i.e., after iterating and adding to
model.retirement_production_processes[(r, p, o)]), so used_techs is only updated
when retirement_production_processes entries are created.
- Around line 607-636: The new p0 EOL branch can add retirement periods for
non-existent existing_capacity rows; update the loop that builds
model.retirement_periods (the unique_rtv iteration handling is_p0_eol) to
require a positive existing_capacity before treating a process as p0 EOL: when
computing is_p0_eol, also check model.existing_capacity[(r,t,v)] (or use
model.existing_capacity.get((r,t,v), 0) if safe) and skip adding p0 retirement
periods if that value is zero or missing; keep all other checks (is_living,
is_natural_eol, is_early_retire, is_survival_curve) unchanged so only real
existing-vintage capacity triggers the p0 exact-EOL branch.
In `@temoa/model_checking/network_model_data.py`:
- Around line 443-448: The branch that sets is_p0_eol currently treats any
vintage whose lifetime ends at the first modeled period as an EOL even if there
was never any installed capacity; update the condition so is_p0_eol also
requires a positive install-base lookup for that (r, tech, v). Concretely,
modify the is_p0_eol assignment (in the loop over periods where is_p0_eol,
is_living, and living_rtv are used) to check the install-base/existing capacity
data structure (e.g., install_base[(r, tech, v)] or
existing_capacity_lookup.get((r,tech,v),0) > 0) before allowing a horizon-start
EOL edge. Ensure the rest of the logic still uses is_p0_eol and is_living
unchanged.
- Around line 357-361: res.demand_commodities is populated from the full payload
before you narrow periods for myopic_index, so in myopic mode you must filter
demand_commodities to the same period slice; after the existing narrowing of
periods (where periods is set from basic_data['periods'] and optionally filtered
by myopic_index.base_year and myopic_index.last_demand_year) remove any entries
in res.demand_commodities whose Period keys fall outside that inclusive range
(use the same Period comparison logic as the periods list filtering) so
demand_commodities aligns with the filtered periods.
---
Outside diff comments:
In `@tests/test_network_model_data.py`:
- Around line 279-286: The two adjacent branches checking for 'technology WHERE
unlim_cap==1' and 'technology WHERE retire==1' both create a MagicMock with
m.fetchall.return_value = [] and return it; replace them with a single
conditional that matches either substring (e.g., using an or or membership
check) and returns the single MagicMock to remove duplication while preserving
behavior in the test helper that inspects the query string.
---
Duplicate comments:
In `@tests/test_network_model_data.py`:
- Around line 367-374: The two separate elif branches checking for 'technology
WHERE unlim_cap==1' and 'technology WHERE retire==1' both create the same
MagicMock with an empty fetchall; replace them with a single branch that detects
either query (e.g., combine the two 'in' checks with an or) and returns one
MagicMock with m.fetchall.return_value = [] to avoid duplicated code; locate the
existing elifs that reference the exact strings 'technology WHERE unlim_cap==1'
and 'technology WHERE retire==1' and merge their logic accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5aebfb27-3df4-4542-9fdc-ab6774be436b
📒 Files selected for processing (5)
temoa/components/capacity.pytemoa/components/technology.pytemoa/core/model.pytemoa/model_checking/network_model_data.pytests/test_network_model_data.py
idelder
force-pushed
the
bugfix/materials_myopic
branch
from
cd16db0 to
fc0d408
Compare
Author
|
Well that was a lot more complicated than I expected |
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.
Materials wasn't playing well with myopic. This fixes some tricky issues around checking viable data for myopic materials databases.
Summary by CodeRabbit
New Features
Bug Fixes
Chores