adding types to utilities#252
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughRemoves Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Migration CLI
participant SourceDB as Source DB
participant Schema as v4 Schema/File
participant TargetDB as Target DB
participant Validator as FK Validator
User->>CLI: run CLI (--source/--input, --schema, --output)
CLI->>SourceDB: open/load source (v3 or v3.1)
CLI->>Schema: load v4 schema
CLI->>TargetDB: create/open target DB and apply schema
loop per source table
CLI->>SourceDB: enumerate table & select rows
CLI->>CLI: map table/column names (token rules)
CLI->>TargetDB: INSERT OR REPLACE mapped columns
end
Note over CLI,TargetDB: Special-case aggregation for capacity_factor_process<br/>and time tables (time_season, time_of_day, sequential)
CLI->>TargetDB: update metadata (DB_MAJOR=4, DB_MINOR=0)
CLI->>TargetDB: VACUUM and enable foreign keys
CLI->>Validator: PRAGMA FOREIGN_KEY_CHECK
Validator-->>CLI: report issues (if any)
CLI->>User: print migration summary
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
temoa/utilities/db_migration_v3_1_to_v4.py (1)
97-101: Consider consolidating duplicateget_table_infofunction.This function is identical to the one in
sql_migration_v3_1_to_v4.py(lines 120-124). Both files also shareCUSTOM_MAP,to_snake_case, andmap_token_no_cascade. Consider extracting these common utilities to a shared module to reduce duplication and ensure consistent behavior.temoa/utilities/unit_cost_explorer.py (1)
157-163: Typo in variable name:mulitpliershould bemultiplier.Proposed fix
-mulitplier: float = ( +multiplier: float = ( storage_dur * model.segment_fraction_per_season[2020, 'winter'] * model.days_per_period * c2a * c ) -print(f'The multiplier for the storage should be: {mulitplier}') +print(f'The multiplier for the storage should be: {multiplier}')
🤖 Fix all issues with AI agents
In @temoa/utilities/unit_cost_explorer.py:
- Around line 141-142: Two duplicate model construction calls are present:
remove the redundant model.v_storage_level.construct() and
model.segment_fraction_per_season.construct() occurrences (the second calls
around the shown diff) because these are already invoked earlier; keep only the
original construct() calls for v_storage_level and segment_fraction_per_season
and delete the repeated lines to avoid unintended double construction.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
pyproject.tomltemoa/utilities/capacity_analyzer.pytemoa/utilities/clear_db_outputs.pytemoa/utilities/database_util.pytemoa/utilities/db_migration_to_v3.pytemoa/utilities/db_migration_v3_1_to_v4.pytemoa/utilities/db_migration_v3_to_v3_1.pytemoa/utilities/graph_utils.pytemoa/utilities/run_all_v4_migrations.pytemoa/utilities/sql_migration_v3_1_to_v4.pytemoa/utilities/unit_cost_explorer.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T15:53:41.829Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 177
File: temoa/model_checking/commodity_network.py:26-33
Timestamp: 2025-10-27T15:53:41.829Z
Learning: The Temoa project requires Python 3.12 or above as the minimum supported version, so PEP 695 `type` syntax for type aliases is appropriate and preferred over `TypeAlias`.
Applied to files:
temoa/utilities/db_migration_to_v3.py
🧬 Code graph analysis (3)
temoa/utilities/db_migration_v3_1_to_v4.py (1)
temoa/utilities/sql_migration_v3_1_to_v4.py (1)
get_table_info(120-124)
temoa/utilities/sql_migration_v3_1_to_v4.py (1)
tests/utilities/namespace_mock.py (1)
Namespace(1-3)
temoa/utilities/capacity_analyzer.py (1)
temoa/utilities/database_util.py (1)
close(59-64)
🪛 Ruff (0.14.10)
temoa/utilities/db_migration_v3_to_v3_1.py
173-173: Possible SQL injection vector through string-based query construction
(S608)
195-195: Possible SQL injection vector through string-based query construction
(S608)
220-220: Possible SQL injection vector through string-based query construction
(S608)
231-233: Possible SQL injection vector through string-based query construction
(S608)
280-280: Possible SQL injection vector through string-based query construction
(S608)
temoa/utilities/db_migration_to_v3.py
139-139: Possible SQL injection vector through string-based query construction
(S608)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
🔇 Additional comments (22)
pyproject.toml (1)
144-144: LGTM!The mypy exclusion pattern update correctly enables type checking for
temoa/utilities/while keepingtemoa/extensions/andstubs/excluded. This aligns with the PR's goal of adding type annotations to the utilities directory.temoa/utilities/clear_db_outputs.py (1)
10-30: LGTM!The type annotations are correctly applied to the module-level variables and local variables. The
list[str]annotations for the table name lists andstrannotations for the command-line argument and user input are appropriate.temoa/utilities/sql_migration_v3_1_to_v4.py (3)
27-27: LGTM!The
Anyimport is appropriate for typing the heterogeneous tuples returned byPRAGMA table_info.
120-124: LGTM!The return type
list[tuple[Any, ...]]accurately reflects thatPRAGMA table_inforeturns tuples with mixed types (cid, name, type, notnull, default_value, pk).
127-127: No test compatibility issue exists. Themigrate_dump_to_sqlitefunction has no tests in the codebase. The customNamespaceclass intests/utilities/namespace_mock.pyis used only intests/test_exchange_cost_ledger.pyfor different functionality, not for testing this function. Additionally, the customNamespaceclass would be compatible with the function's interface due to its duck-typed attribute access pattern (the function only accessesargs.input,args.schema,args.output, andargs.debug).temoa/utilities/db_migration_v3_1_to_v4.py (2)
19-19: LGTM!The
Anyimport is needed for the heterogeneous tuple typing.
135-135: LGTM!The
argparse.Namespacetype annotation correctly specifies the expected parameter type for CLI-driven execution.temoa/utilities/capacity_analyzer.py (3)
44-57: Defensive resource cleanup is appropriate for this script.The type annotations are correct. The
if 'con' in locals()pattern works for script-style code sinceconis only defined ifsqlite3.connectsucceeds. Note that the relevant snippet fromdatabase_util.pyuseshasattr(self, 'con')for class-based cleanup—both patterns are appropriate for their respective contexts.
60-82: LGTM!The type annotations for the data processing variables (
caps,small_cap_sources,large_cap_sources,total_cap,cumulative_caps) are accurate and improve code clarity.
23-41: Robust package resource discovery with correct fallback.The use of
importlib.resources.filesfor locating data files is a good improvement for package installations. The fallback path correctly handles the actual repository structure where the script is attemoa/utilities/capacity_analyzer.pyanddata_filesis at the repository root, usingcurrent_file.parents[2]to traverse up to the root level.temoa/utilities/unit_cost_explorer.py (1)
6-7: Type annotations look good.The added type hints improve code clarity and type safety. The use of
cast('Any', ...)for Pyomo model dynamic attributes is appropriate since Pyomo uses runtime attribute assignment that mypy cannot statically verify.Also applies to: 34-35, 71-76, 79-80, 87-96
temoa/utilities/db_migration_to_v3.py (2)
248-248: Good fix for operator precedence.The parentheses in
not (common ^ techs)correctly ensure the symmetric difference is computed before negation. The previous expressionnot common ^ techswould have been interpreted as(not common) ^ techs, which would produce incorrect results.
40-42: Type annotations improve code clarity.The added type hints for
sqlite3.Connection,sqlite3.Cursor, and the various list/dict structures are appropriate and improve maintainability.Regarding the static analysis SQL injection warnings (S608): These are false positives in this context. This is an internal migration tool that constructs queries using trusted table/column names from the database schema itself, not from external user input. The dynamic SQL is necessary for the table migration logic.
Also applies to: 54-56, 109-120, 130-143, 230-234, 366-372
temoa/utilities/db_migration_v3_to_v3_1.py (3)
14-15: Improved import pattern.Assuming
temoais installed in the environment is cleaner than the previous dynamicsys.pathmanipulation. This aligns with standard Python packaging practices.
278-316: Pandas usage for period indexing is appropriate.Using
pd.DataFramefor the complex period calculation logic improves readability compared to pure list/dict manipulation. The typing withpd.DataFrameand proper guards (len(df_data) == 0) are well-structured.
39-41: Type annotations are consistent and well-applied.The variable renaming from generic
datato more descriptive names likedata_rows,data_transformed, anddf_dataimproves code clarity. The explicit type hints forlist[Any],dict[...], andsqlite3.*types are appropriate.Regarding SQL injection warnings (S608): These are false positives. This migration script uses table and column names from the database schema itself, not external user input.
Also applies to: 78-120, 173-196, 238-260, 373-395, 423-430
temoa/utilities/run_all_v4_migrations.py (1)
26-36: Accurate return type annotation.The
subprocess.CompletedProcess[str]return type is correct since the function passestext=Truetosubprocess.run(), makingstdoutandstderrreturnstrinstead ofbytes.temoa/utilities/database_util.py (3)
59-64: Good defensive programming inclose()method.The
hasattrguards handle edge cases where the object may not be fully initialized (e.g., if an exception occurs during__init__beforeconorcurare assigned). This preventsAttributeErrorduring cleanup.
133-133: Appropriate use ofcast()for type consistency.The
cast()calls ensure the return types match the declared signatures (set[str],set[tuple[str, str]]) when SQLite returns generic row tuples. This is a clean way to bridge the gap between SQLite's dynamic typing and Python's static type hints.Also applies to: 179-179, 195-195
197-199: Return type accurately reflects method behavior.The widened return type
pd.DataFrame | pd.Series[Any]correctly reflects that the method returns aSeries(viaresult.sum()on line 229) when certain conditions are met, and aDataFrameotherwise.temoa/utilities/graph_utils.py (2)
40-46: Multi-line TypeVar formatting is consistent.The reformatted
GraphTypeTypeVar maintains the same type constraints while improving readability. The runtime definition (without generic parameters) is necessary because NetworkX graph types cannot be subscripted at runtime.
49-54: Function signature updated withverbosityparameter.The
noqa: UP047directive is appropriate to suppress the Ruff rule that suggests using PEP 695typesyntax, which would cause runtime errors with NetworkX's generic graph types.The new
verbosityparameter enables conditional debug logging (used on lines 72 and 98). All callers intemoa/utilities/visualizer.pyhave been updated to pass theverbosityargument (passing 0 in both cases).
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
from
1cd2091 to
ebc2516
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/utilities/database_util.py (1)
112-123: Consider fixing the method name typo.The method is named
get_time_peridos_for_flagsbut should beget_time_periods_for_flags. This appears to be pre-existing, but worth fixing while you're updating this file.The
int()cast with explanatory comment is a good addition.
🤖 Fix all issues with AI agents
In @temoa/utilities/capacity_analyzer.py:
- Around line 55-57: The cleanup uses a non-idiomatic "if 'con' in locals()"
check; instead initialize the connection variable (con = None) before the try in
the function/method that creates the DB connection and then in the finally block
check "if con is not None: con.close()"; update references to the connection
variable (con) accordingly so the finally block reliably closes only an actual
connection.
- Around line 23-41: Update the misleading fallback comment near the fallback
block that builds source_db_file: clarify that current_file =
Path(__file__).resolve() refers to temoa/utilities/capacity_analyzer.py and that
.parents[2] climbs to the repository root where data_files/untracked_data/db
resides (i.e., "# temoa/utilities/capacity_analyzer.py -> data_files (at repo
root)"). Keep the existing .parents[2] logic and variable names (current_file,
source_db_file) unchanged.
In @temoa/utilities/database_util.py:
- Line 133: The cast usage in the return expression of the method that does
"return {cast('str', row[0]) for row in self.cur.execute(query)}" should use a
real type object instead of a string literal; change the cast call to cast(str,
row[0]) (no quotes) in that set comprehension so it uses the actual type for
Python 3.12+ type narrowing while leaving the rest of the expression
(self.cur.execute(query)) unchanged.
In @temoa/utilities/db_migration_v3_to_v3_1.py:
- Around line 244-254: The lifetime_process annotation is overly broad; change
its type from dict[tuple[Any, ...], float] to a more specific tuple type
reflecting the keys used (region, tech, vintage). Update the declaration of
lifetime_process to use either dict[tuple[str, str, int], float] if vintage is
an int, or dict[tuple[str, str, Any], float] if vintage can vary, keeping use of
TemoaModel.default_lifetime_tech and the subsequent assignments unchanged.
- Around line 283-285: Replace the idiomatic DataFrame emptiness check: in the
block that currently does "if len(df_data) == 0: print('No data for: ' +
old_name) continue", use the pandas property "df_data.empty" instead (i.e., "if
df_data.empty: ...") so the check is clearer and more idiomatic while preserving
the existing print and continue behavior; locate the snippet referencing df_data
and old_name and make the swap.
In @temoa/utilities/unit_cost_explorer.py:
- Around line 38-39: Replace the inline tuple type annotations for the recurring
index tuples with PEP 695 type aliases: create type RTV = tuple[str, str, int]
and type RPTV = tuple[str, int, str, int], then change the variable annotations
for rtv and rptv to use RTV and RPTV respectively so the intent is clearer and
the aliases can be reused across the module.
- Around line 142-146: The two calls to model.v_storage_level.construct() and
model.segment_fraction_per_season.construct() are duplicated; remove the
redundant second occurrences so each of model.v_storage_level.construct() and
model.segment_fraction_per_season.construct() is invoked only once (keep the
first calls and delete the repeated calls that follow).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
pyproject.tomltemoa/utilities/capacity_analyzer.pytemoa/utilities/clear_db_outputs.pytemoa/utilities/database_util.pytemoa/utilities/db_migration_to_v3.pytemoa/utilities/db_migration_v3_1_to_v4.pytemoa/utilities/db_migration_v3_to_v3_1.pytemoa/utilities/graph_utils.pytemoa/utilities/run_all_v4_migrations.pytemoa/utilities/sql_migration_v3_1_to_v4.pytemoa/utilities/unit_cost_explorer.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 177
File: temoa/model_checking/commodity_network.py:26-33
Timestamp: 2025-10-27T15:53:41.829Z
Learning: The Temoa project requires Python 3.12 or above as the minimum supported version, so PEP 695 `type` syntax for type aliases is appropriate and preferred over `TypeAlias`.
🧬 Code graph analysis (4)
temoa/utilities/db_migration_v3_1_to_v4.py (2)
temoa/utilities/sql_migration_v3_1_to_v4.py (1)
get_table_info(120-124)tests/utilities/namespace_mock.py (1)
Namespace(1-3)
temoa/utilities/capacity_analyzer.py (1)
temoa/utilities/database_util.py (1)
close(59-64)
temoa/utilities/unit_cost_explorer.py (1)
temoa/components/storage.py (1)
storage_energy_upper_bound_constraint(219-273)
temoa/utilities/database_util.py (1)
temoa/utilities/graphviz_generator.py (1)
close(189-194)
🪛 Ruff (0.14.10)
temoa/utilities/db_migration_v3_to_v3_1.py
173-173: Possible SQL injection vector through string-based query construction
(S608)
195-195: Possible SQL injection vector through string-based query construction
(S608)
220-220: Possible SQL injection vector through string-based query construction
(S608)
231-233: Possible SQL injection vector through string-based query construction
(S608)
280-280: Possible SQL injection vector through string-based query construction
(S608)
temoa/utilities/db_migration_to_v3.py
139-139: Possible SQL injection vector through string-based query construction
(S608)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
- GitHub Check: setup and test (macos-latest, 3.12)
🔇 Additional comments (36)
temoa/utilities/graph_utils.py (2)
40-46: LGTM!The multi-line formatting with trailing commas improves readability and maintains consistency with the
TYPE_CHECKINGblock above.
49-54: Thenoqa: UP047suppression is correct.UP047 (
non-pep695-generic-function) is a valid ruff rule that suggests converting standalone TypeVar/ParamSpec/TypeVarTuple style generics to PEP 695 inline type-parameter syntax. The suppression is appropriate here to preserve networkx compatibility, as converting theGraphTypeparameter to PEP 695 syntax could cause runtime type errors with networkx generics.pyproject.toml (1)
144-144: LGTM!The narrowed exclusion pattern correctly enables type checking for
temoa/utilities/while maintaining exclusions fortemoa/extensions/andstubs/. This aligns with the PR objective of adding typing to the utilities subdirectory.temoa/utilities/run_all_v4_migrations.py (1)
26-36: LGTM!The return type
subprocess.CompletedProcess[str]is correct sincetext=Trueis passed tosubprocess.run, ensuring stdout/stderr are strings rather than bytes.temoa/utilities/clear_db_outputs.py (1)
10-30: LGTM!The type annotations are correct and use modern Python 3.12+ syntax (
list[str]instead ofList[str]), which is appropriate for this project. Based on learnings, Python 3.12+ is the minimum supported version.temoa/utilities/sql_migration_v3_1_to_v4.py (3)
27-27: LGTM!The
Anyimport is correctly added to support the return type annotation.
120-124: LGTM!The return type
list[tuple[Any, ...]]is the correct annotation for SQLite'sPRAGMA table_inforesults, which returns tuples with heterogeneous element types (cid, name, type, notnull, default_value, pk).
127-127: LGTM!Adding the
argparse.Namespacetype annotation improves type safety for the function parameter.temoa/utilities/unit_cost_explorer.py (4)
6-16: LGTM!The
TYPE_CHECKINGguard pattern is correctly used to import type hints without runtime overhead. This is a clean approach for importing types fromtemoa.types.core_types.
148-157: Casting string/int literals to domain types is verbose but acceptable.The multiple
cast()calls for literal values likecast('Region', 'A')work for satisfying mypy but are quite verbose. Since this is a QA/exploration script rather than production code, this approach is acceptable. An alternative would be to define typed variables once and reuse them.
71-84: LGTM!The use of
cast('Any', ...)for mutable model attributes and explicit type annotations fortot_cost_exprandtotal_costproperly satisfy the type checker while maintaining the existing runtime behavior.
91-100: LGTM!The explicit
floattype annotations for the storage calculation variables improve clarity and type safety. The unit conversion calculations are well-documented through variable naming.temoa/utilities/db_migration_v3_1_to_v4.py (3)
19-19: LGTM!The
Anyimport is correctly added to support the typed return value ofget_table_info.
97-101: LGTM!The return type
list[tuple[Any, ...]]correctly represents the heterogeneous PRAGMA table_info result and is consistent with the same function insql_migration_v3_1_to_v4.py.
135-135: LGTM!Explicit
argparse.Namespacetype annotation correctly documents the expected input type fromparse_cli().temoa/utilities/capacity_analyzer.py (3)
9-10: LGTM!Imports correctly added to support
Pathoperations andAnytype annotation.
60-67: LGTM!Type annotations for the data processing pipeline are accurate and improve code clarity.
79-82: LGTM!Cumulative capacity calculation is correctly typed.
temoa/utilities/database_util.py (5)
10-16: LGTM!Future annotations and typing imports correctly added to support enhanced type hints throughout the module.
59-64: LGTM!The
hasattrguards correctly protect againstAttributeErrorif the constructor fails before initializingcurorcon.
179-179: LGTM!Same pattern as above - cast correctly narrows the type for the type checker.
195-195: LGTM!Cast correctly specifies the tuple type for commodity-technology pairs.
197-199: LGTM!The union return type
pd.DataFrame | pd.Series[Any]correctly reflects that the method can return either type based on the conditional logic at lines 228-231.temoa/utilities/db_migration_v3_to_v3_1.py (7)
9-15: LGTM!The typing import and direct import from
temoa.core.model(assuming installed package) is cleaner than dynamic path manipulation.
39-41: LGTM!Connection and cursor type annotations are correct and consistent with the other migration scripts.
78-156: LGTM!Type annotations for table mappings are accurate and improve code readability.
168-197: LGTM with note on SQL construction.The SQL queries use f-strings with table names from the hardcoded
operator_added_tablesdictionary (not user input), so the static analysis SQL injection warning is a false positive in this context.The defensive empty-data checks on lines 178-180 and 191-193 are good additions.
215-236: LGTM!Type annotations correctly added. The SQL injection warnings are false positives since table/column names come from the hardcoded configuration.
373-395: LGTM!Defensive handling for missing LoanLifetimeTech table with proper fallback to empty list.
423-430: LGTM!FK validation with proper type annotations and error handling.
temoa/utilities/db_migration_to_v3.py (6)
15-15: LGTM!
Anyimport added to support type annotations.
40-42: LGTM!Connection and cursor type annotations are consistent with the other migration scripts in this PR.
54-120: LGTM!Type annotations for table mappings are accurate.
130-143: LGTM!Column and data fetching with proper type annotations. The SQL injection warning is a false positive since table names come from the hardcoded configuration, not user input.
230-248: LGTM!The
rps_entriestype annotation is correct. The boolean logic on line 248 correctly tracks whether all non-global groups share the same technologies by checking if the symmetric difference is empty.
366-366: LGTM!Type annotation for
new_datais correct.
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
from
ebc2516 to
7134677
Compare
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 (4)
temoa/utilities/sql_migration_v3_1_to_v4.py (1)
29-50: Consider extracting shared mapping logic.The
CUSTOM_MAP,CUSTOM_EXACT_ONLY,CUSTOM_KEYS_SORTED,to_snake_case,map_token_no_cascade, andget_table_infoare duplicated between this file anddb_migration_v3_1_to_v4.py. Extracting these into a shared module would reduce maintenance burden and ensure consistency.temoa/utilities/clear_db_outputs.py (1)
39-42: Consider using f-strings for consistency.The rest of the file uses f-strings (line 34), but these SQL statements use string concatenation. Consider using f-strings for stylistic consistency.
♻️ Suggested change
for table in basic_output_tables: - conn.execute('DELETE FROM ' + table + ' WHERE 1') + conn.execute(f'DELETE FROM {table} WHERE 1') for table in optional_output_tables: try: - conn.execute('DELETE FROM ' + table + ' WHERE 1') + conn.execute(f'DELETE FROM {table} WHERE 1') except sqlite3.OperationalError: passtemoa/utilities/unit_cost_explorer.py (1)
161-168: Typo in variable name:mulitplier→multiplier.The variable name has a spelling error.
📝 Fix typo
-mulitplier: float = ( +multiplier: float = ( storage_dur * model.segment_fraction_per_season[2020, 'winter'] * model.days_per_period * c2a * c ) -print(f'The multiplier for the storage should be: {mulitplier}') +print(f'The multiplier for the storage should be: {multiplier}')temoa/utilities/db_migration_to_v3.py (1)
138-143: Redundant assignment beforecontinue.Setting
data = []on line 142 is immediately followed bycontinue, so the assignment has no effect.♻️ Remove redundant assignment
try: data: list[Any] = con_old.execute(f'SELECT {cols} FROM {old_name}').fetchall() except sqlite3.OperationalError: print('TABLE NOT FOUND: ' + old_name) - data = [] continue
🤖 Fix all issues with AI agents
In @temoa/utilities/capacity_analyzer.py:
- Around line 36-37: The current fragile cleanup uses "'con' in locals()" which
is error-prone; update the code that opens the DB
(sqlite3.connect(source_db_file)) and the cursor usage (con.cursor(), cur) to
use a context manager or ensure deterministic cleanup: replace manual locals()
check with "with sqlite3.connect(source_db_file) as con:" and perform cur =
con.cursor() and queries inside that block, handling sqlite3.Error in an except
clause, or if you prefer explicit try/finally ensure con is assigned before
calling con.cursor() and in finally check "if con is not None: con.close()";
reference the sqlite3.connect, con, cur, and sqlite3.Error symbols when making
the change.
In @temoa/utilities/database_util.py:
- Line 195: The code unnecessarily constructs a new tuple with tuple(row) when
rows from self.cur.execute(query) are already tuples; modify the return in the
method that contains self.cur.execute(query) to cast the existing row directly
(e.g., use cast('tuple[str, str]', row) in the set comprehension) instead of
wrapping row in tuple(...), removing the redundant tuple construction.
In @temoa/utilities/unit_cost_explorer.py:
- Around line 145-147: The two construct() calls for
model.v_storage_level.construct() and
model.segment_fraction_per_season.construct() are duplicated; remove the second
set of calls (the repeated model.v_storage_level.construct() and
model.segment_fraction_per_season.construct()) so each construct() is only
invoked once (keep the first occurrences) to avoid potential non-idempotent side
effects.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
pyproject.tomltemoa/utilities/capacity_analyzer.pytemoa/utilities/clear_db_outputs.pytemoa/utilities/database_util.pytemoa/utilities/db_migration_to_v3.pytemoa/utilities/db_migration_v3_1_to_v4.pytemoa/utilities/db_migration_v3_to_v3_1.pytemoa/utilities/graph_utils.pytemoa/utilities/run_all_v4_migrations.pytemoa/utilities/sql_migration_v3_1_to_v4.pytemoa/utilities/unit_cost_explorer.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 177
File: temoa/model_checking/commodity_network.py:26-33
Timestamp: 2025-10-27T15:53:41.829Z
Learning: The Temoa project requires Python 3.12 or above as the minimum supported version, so PEP 695 `type` syntax for type aliases is appropriate and preferred over `TypeAlias`.
📚 Learning: 2025-10-27T15:53:41.829Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 177
File: temoa/model_checking/commodity_network.py:26-33
Timestamp: 2025-10-27T15:53:41.829Z
Learning: The Temoa project requires Python 3.12 or above as the minimum supported version, so PEP 695 `type` syntax for type aliases is appropriate and preferred over `TypeAlias`.
Applied to files:
temoa/utilities/db_migration_to_v3.pytemoa/utilities/unit_cost_explorer.py
🧬 Code graph analysis (4)
temoa/utilities/sql_migration_v3_1_to_v4.py (2)
temoa/utilities/db_migration_v3_1_to_v4.py (1)
get_table_info(97-101)tests/utilities/namespace_mock.py (1)
Namespace(1-3)
temoa/utilities/db_migration_v3_1_to_v4.py (2)
temoa/utilities/sql_migration_v3_1_to_v4.py (1)
get_table_info(120-124)tests/utilities/namespace_mock.py (1)
Namespace(1-3)
temoa/utilities/database_util.py (1)
temoa/utilities/graphviz_generator.py (1)
close(189-194)
temoa/utilities/capacity_analyzer.py (2)
temoa/utilities/graphviz_generator.py (1)
connect(173-187)temoa/utilities/database_util.py (1)
close(59-64)
🪛 Ruff (0.14.10)
temoa/utilities/db_migration_to_v3.py
139-139: Possible SQL injection vector through string-based query construction
(S608)
temoa/utilities/db_migration_v3_to_v3_1.py
173-173: Possible SQL injection vector through string-based query construction
(S608)
195-195: Possible SQL injection vector through string-based query construction
(S608)
220-220: Possible SQL injection vector through string-based query construction
(S608)
231-233: Possible SQL injection vector through string-based query construction
(S608)
280-280: Possible SQL injection vector through string-based query construction
(S608)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
🔇 Additional comments (23)
pyproject.toml (1)
144-144: LGTM! Enabling type checking for utilities.The mypy exclude pattern now correctly includes only
temoa/extensions/andstubs/, enabling type checking for thetemoa/utilities/package as intended by this PR.temoa/utilities/run_all_v4_migrations.py (1)
26-28: LGTM! Accurate generic type annotation.The return type
subprocess.CompletedProcess[str]correctly reflects the use oftext=Truein thesubprocess.runcall, ensuring the generic parameter matches the actual output type.temoa/utilities/sql_migration_v3_1_to_v4.py (3)
27-27: LGTM! Type import added.The
Anyimport is necessary for thelist[tuple[Any, ...]]return type annotation.
120-124: LGTM! Appropriate return type for PRAGMA result.
list[tuple[Any, ...]]correctly represents the variable-length tuple rows returned byPRAGMA table_info.
127-127: LGTM! Proper type annotation for CLI args.Using
argparse.Namespacecorrectly types the parsed command-line arguments.temoa/utilities/db_migration_v3_1_to_v4.py (3)
19-19: LGTM! Type import added.The
Anyimport supports the improved return type annotation.
97-101: LGTM! Consistent return type annotation.The return type
list[tuple[Any, ...]]correctly matches the corresponding function insql_migration_v3_1_to_v4.pyand accurately represents PRAGMA results.
135-135: LGTM! Proper type annotation for CLI args.Using
argparse.Namespaceis correct and consistent with the other migration script.temoa/utilities/clear_db_outputs.py (2)
10-22: LGTM! Type annotations for module-level lists.The
list[str]annotations correctly type the output table name collections.
28-30: LGTM! Type annotations for string variables.The
strannotations are straightforward and accurate for the CLI argument and user input.temoa/utilities/capacity_analyzer.py (1)
59-62: Type annotation forcumulative_capsinitialization is good.The explicit typing for
total_cap: floatandcumulative_caps: list[float]improves clarity. The initialization logic correctly handles the first element before iterating.temoa/utilities/graph_utils.py (2)
40-46: LGTM!The reformatted
TypeVardeclaration with trailing commas improves readability and follows modern Python formatting conventions.
49-54: LGTM!The
noqa: UP047directive is appropriately documented to explain why it's avoiding a runtime type error with NetworkX generics. The function signature is well-typed.temoa/utilities/database_util.py (2)
61-64: LGTM!The
hasattrguards provide defensive protection against attribute errors if the constructor fails partway through. This aligns with the pattern shown in the relevant code snippet fromgraphviz_generator.py.
121-123: Good defensive casting.Explicitly casting to
intensures type consistency regardless of how SQLite returns the data. The comment explains the rationale clearly.temoa/utilities/db_migration_v3_to_v3_1.py (4)
14-15: Import assumption may need verification.The comment states "Assumes temoa is installed in the environment." This is a reasonable assumption for a migration script, but it changes from a previous path-based import approach.
Ensure that when this migration script is run,
temoais properly installed in the environment, or document this requirement in the script's usage instructions.
39-41: LGTM!Explicit type annotations for
sqlite3.Connectionandsqlite3.Cursorimprove code clarity and enable better static analysis.
278-281: Good use of pandas for complex data extraction.Using
pd.read_sql_querysimplifies the data loading and provides a cleaner interface for the subsequent DataFrame operations.
173-197: SQL injection note for migration script.Static analysis flags SQL injection risk due to string-based query construction. While this is a CLI migration tool operating on the user's own databases (low risk), consider using parameterized queries where feasible for defense in depth.
The type annotations and empty-data guards (
if not data_transformed: continue) are well implemented.⛔ Skipped due to learnings
Learnt from: ParticularlyPythonicBS Repo: TemoaProject/temoa PR: 184 File: temoa/data_processing/database_util.py:109-118 Timestamp: 2025-11-03T13:34:35.907Z Learning: In the Temoa project, SQL injection vulnerabilities in database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during code reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.Learnt from: ParticularlyPythonicBS Repo: TemoaProject/temoa PR: 184 File: temoa/data_processing/database_util.py:109-118 Timestamp: 2025-11-03T13:34:35.907Z Learning: In the Temoa project, SQL injection vulnerabilities in the database utility classes (such as temoa/data_processing/database_util.py) are considered low concern due to the data access patterns and should not be flagged during reviews. Technical debt for SQL injection hardening should be tracked separately as issues rather than blocking PRs.temoa/utilities/unit_cost_explorer.py (1)
14-16: LGTM!The
TYPE_CHECKINGblock correctly guards the type-only imports to avoid runtime overhead. Based on learnings, PEP 695 style could be used here, but the current approach is also valid.temoa/utilities/db_migration_to_v3.py (3)
40-42: LGTM!Explicit type annotations for database connections and cursor improve code clarity and static analysis support.
248-248: Important operator precedence fix.Adding parentheses around
(common ^ techs)is a correctness fix. Without parentheses,not common ^ techswould evaluate as(not common) ^ techsdue to Python's operator precedence (notbinds tighter than^). The intended logic isnot (common ^ techs)to check if the symmetric difference is empty.
230-234: LGTM!Good defensive handling - initializing
rps_entriesto an empty list when the table is not found ensures the subsequent iteration doesn't fail.
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
from
7134677 to
8c63457
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/utilities/unit_cost_explorer.py (1)
176-183: Typo:mulitpliershould bemultiplier.Proposed fix
-mulitplier: float = ( +multiplier: float = ( storage_dur * model.segment_fraction_per_season[2020, 'winter'] * model.days_per_period * c2a * c ) -print(f'The multiplier for the storage should be: {mulitplier}') +print(f'The multiplier for the storage should be: {multiplier}')
🤖 Fix all issues with AI agents
In @temoa/utilities/capacity_analyzer.py:
- Around line 55-58: Guard against an empty or all-zero caps list before
computing normalized cumulative capacities: check if caps is empty or if
total_cap := sum(caps) is 0 and handle by returning an appropriate empty/zero
result (e.g., return [] or cumulative_caps filled with zeros) instead of
performing caps[0] / total_cap; update the code around the variables caps,
total_cap, and cumulative_caps to perform this early-return or conditional logic
and only compute the normalized cumulative list when total_cap > 0.
In @temoa/utilities/unit_cost_explorer.py:
- Around line 163-172: The call to storage_energy_upper_bound_constraint uses
cast('TimeOfDay', '1') passing a string where TimeOfDay values are integers;
change that argument to an integer (e.g., cast('TimeOfDay', 1) or simply 1
depending on your cast helper) so it matches the TimeOfDay set created with
range(1, tod_slices + 1); update the call site near model.is_seasonal_storage
and storage_energy_upper_bound_constraint to pass an integer TimeOfDay value
instead of the string '1'.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
stubs/pyomo/core/base/component.pyiis excluded by!stubs/**
📒 Files selected for processing (11)
pyproject.tomltemoa/utilities/capacity_analyzer.pytemoa/utilities/clear_db_outputs.pytemoa/utilities/database_util.pytemoa/utilities/db_migration_to_v3.pytemoa/utilities/db_migration_v3_1_to_v4.pytemoa/utilities/db_migration_v3_to_v3_1.pytemoa/utilities/graph_utils.pytemoa/utilities/run_all_v4_migrations.pytemoa/utilities/sql_migration_v3_1_to_v4.pytemoa/utilities/unit_cost_explorer.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T15:53:41.829Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 177
File: temoa/model_checking/commodity_network.py:26-33
Timestamp: 2025-10-27T15:53:41.829Z
Learning: The Temoa project requires Python 3.12 or above as the minimum supported version, so PEP 695 `type` syntax for type aliases is appropriate and preferred over `TypeAlias`.
Applied to files:
temoa/utilities/db_migration_v3_to_v3_1.pytemoa/utilities/unit_cost_explorer.py
🧬 Code graph analysis (3)
temoa/utilities/sql_migration_v3_1_to_v4.py (2)
temoa/utilities/db_migration_v3_1_to_v4.py (1)
get_table_info(97-101)tests/utilities/namespace_mock.py (1)
Namespace(1-3)
temoa/utilities/db_migration_v3_to_v3_1.py (1)
temoa/core/model.py (1)
TemoaModel(93-1137)
temoa/utilities/unit_cost_explorer.py (3)
temoa/components/costs.py (1)
total_cost_rule(522-664)temoa/components/storage.py (1)
storage_energy_upper_bound_constraint(219-273)temoa/extensions/monte_carlo/mc_run.py (1)
model(185-192)
🪛 Ruff (0.14.10)
temoa/utilities/db_migration_v3_to_v3_1.py
173-173: Possible SQL injection vector through string-based query construction
(S608)
195-195: Possible SQL injection vector through string-based query construction
(S608)
220-220: Possible SQL injection vector through string-based query construction
(S608)
231-233: Possible SQL injection vector through string-based query construction
(S608)
280-280: Possible SQL injection vector through string-based query construction
(S608)
temoa/utilities/db_migration_to_v3.py
139-139: Possible SQL injection vector through string-based query construction
(S608)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.13)
🔇 Additional comments (37)
pyproject.toml (1)
144-144: LGTM!Removing
temoa/utilities/from the mypy exclusion list correctly enables type checking for the utilities package, aligning with the typing additions in this PR.temoa/utilities/unit_cost_explorer.py (4)
6-24: LGTM!Good use of
TYPE_CHECKINGguard to avoid runtime import overhead for type-only imports, andfrom __future__ import annotationsenables postponed evaluation of annotations. This is the recommended pattern for the project per the learnings (Python 3.12+).
47-57: LGTM!The
cast()calls correctly narrow string literals to the appropriateNewTypealiases for type checking purposes without any runtime overhead.
93-118: LGTM!The explicit type annotations for local variables (
v_new_capacity,v_capacity,total_cost,storage_cap,storage_dur,c2a,c,storage,PJ_to_kwh,price_per_kwh) improve clarity and enable mypy to verify the calculations.
128-146: LGTM!The type annotations for
tod_slices: intandseasonal_fractions: dict[str, float]are correct and consistent with their usage.temoa/utilities/run_all_v4_migrations.py (1)
26-36: LGTM!The return type
subprocess.CompletedProcess[str]is correct sincetext=Trueis passed tosubprocess.run, ensuringstdoutandstderrare decoded as strings.temoa/utilities/db_migration_v3_1_to_v4.py (3)
19-19: LGTM!The
Anyimport is correctly added to support the generic tuple type annotation.
97-101: LGTM!The return type
list[tuple[Any, ...]]accurately reflects thatPRAGMA table_inforeturns rows with varying column types. The empty list return onOperationalErroris a sensible fallback.
135-135: LGTM!Typing the
argsparameter asargparse.Namespaceis correct and consistent with theparse_cli()return type.temoa/utilities/sql_migration_v3_1_to_v4.py (3)
27-27: LGTM!The
Anyimport is correctly added to support the generic tuple type annotation, consistent withdb_migration_v3_1_to_v4.py.
120-124: LGTM!The return type
list[tuple[Any, ...]]matches the implementation indb_migration_v3_1_to_v4.py, maintaining consistency across the migration utilities.
127-127: LGTM!Typing
argsasargparse.Namespaceis correct and aligns with the pattern established in the sibling migration file.temoa/utilities/clear_db_outputs.py (2)
10-22: LGTM!The type annotations for
basic_output_tablesandoptional_output_tablesare correct and use the modernlist[str]syntax appropriate for Python 3.12+.
28-30: LGTM!Type annotations for command-line argument and user input are appropriate.
temoa/utilities/capacity_analyzer.py (2)
24-30: LGTM!Good use of context manager for database connection, which ensures proper cleanup. The type annotations for
res,cur, and iteration are appropriate.
36-43: LGTM!Type annotations for the capacity processing logic are clear and correctly typed.
temoa/utilities/graph_utils.py (2)
40-46: LGTM!The multi-line formatting of the
GraphTypeTypeVar in the runtime branch improves readability while maintaining the same type constraints as theTYPE_CHECKINGbranch.
49-54: LGTM!The
noqa: UP047directive is appropriate here to avoid runtime type errors with NetworkX generics. The function signature is well-typed with clear parameter and return type annotations.temoa/utilities/database_util.py (5)
61-64: LGTM!The
hasattrguards are a good defensive pattern to handle cases where__init__might fail before settingcurorconattributes. This preventsAttributeErrorduring cleanup.
121-123: LGTM!The explicit cast to
intwith the explanatory comment is appropriate since SQLite can return different types depending on how data was inserted.
133-133: LGTM!Using
cast('str', row[0])appropriately narrows the type from the generic cursor result to satisfy theset[str]return type.
179-179: LGTM!Consistent use of
castfor type narrowing, matching the pattern used inget_technologies_for_flags.
195-199: LGTM!The cast to
tuple[str, str]and the broadened return typepd.DataFrame | pd.Series[Any]accurately reflect the runtime behavior whereresult.sum()returns a Series.temoa/utilities/db_migration_v3_to_v3_1.py (8)
39-41: LGTM!Explicit type annotations for the database connections and cursor improve code clarity.
78-111: LGTM!The typed table mappings (
list[tuple[str, str]]) clearly document the structure of the migration configuration.
121-156: LGTM!Type annotations for
operator_added_tablesandno_transferdictionaries are appropriate and self-documenting.
186-197: Verify tuple slicing logic for operator insertion.The tuple construction
(*row[0:op_index], operator, *row[op_index : len(new_cols) - 1])inserts the operator at the correct position, but the end slicelen(new_cols) - 1assumes the old row has one fewer column than the new schema. This should work correctly given the migration context, but verify that source tables consistently have exactly one less column than the destination.
244-254: LGTM!The lifetime calculation logic with typed dictionary
lifetime_process: dict[tuple[Any, ...], float]is clear. The cascading precedence (default → LifetimeTech → LifetimeProcess) correctly handles lifetime overrides.
279-316: LGTM!Using pandas DataFrame for complex period calculations is appropriate. The guards for empty data (
len(df_data) == 0) and the conditional period assignment based on column presence are well-structured.
373-395: LGTM!Good defensive pattern: catching
OperationalErrorand initializingdata_rows = []ensures graceful handling when theLoanLifetimeTechtable doesn't exist in the source database.
423-430: LGTM!The FK validation with proper error handling provides useful migration diagnostics.
temoa/utilities/db_migration_to_v3.py (6)
15-15: LGTM!Type annotations for the SQLite connections and cursor are correct and improve code clarity.
Also applies to: 40-42
54-54: LGTM!Type annotations for table mappings correctly specify
list[tuple[str, str]], accurately reflecting the structure of old-to-new table name pairs.Also applies to: 109-109, 116-116
130-143: Type annotations are correct; SQL injection warning is a false positive.The static analysis tool flags line 139 for SQL injection (S608), but this is safe because:
- Table names (
old_name) come from the hardcodeddirect_transfer_tableslist- Column names come from
PRAGMA table_infoon those tables (trusted schema metadata)- SQLite doesn't support parameterized identifiers (table/column names), so string construction is the standard approach for migration scripts
Note: The
data = []assignment on line 142 is immediately followed bycontinue, making it dead code. It's harmless but could be removed for clarity.
230-234: LGTM!The type annotation
list[tuple[str, str, str]]correctly describes the RPS entry structure (region, tech, notes). Initializingrps_entries = []in the error path ensures the variable is always defined, which satisfies type checkers even though the guard at line 235 prevents its use whenskip_rpsis True.
244-253: Good catch: Parentheses fix operator precedence bug.The original expression
not common ^ techswas evaluated as(not common) ^ techsdue to Python's operator precedence (notbinds tighter than^), which would incorrectly XOR a boolean with a set.The corrected
not (common ^ techs)properly:
- Computes the symmetric difference (mismatches between sets)
- Returns
Trueonly when there are no mismatchesThis is a critical correctness fix.
364-372: LGTM!The
list[Any]annotation is appropriate here since the row tuples contain heterogeneous data types. The null-to-zero conversion forunlim_capis correctly handled.
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
from
8c63457 to
b928b58
Compare
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
from
b928b58 to
980dae7
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 (2)
temoa/utilities/graph_utils.py (1)
55-55: 🧹 Nitpick | 🔵 TrivialConsider documenting the
verbosityparameter.The docstring should describe the new
verbosityparameter and its expected values (e.g., what verbosity level triggers debug logging).📝 Suggested docstring update
def convert_graph_to_json( # noqa: UP047 # avoiding runtime type error with networkx generics nx_graph: GraphType, override_node_properties: dict[str, Any] | None, override_edge_properties: dict[str, Any] | None, verbosity: int, ) -> tuple[list[dict[str, Any]], list[dict[str, Any]]]: - """Helper to convert a single NetworkX graph to JSON-serializable lists.""" + """Helper to convert a single NetworkX graph to JSON-serializable lists. + + Args: + nx_graph: The NetworkX graph to convert. + override_node_properties: Properties to apply to all nodes (excluding 'id'). + override_edge_properties: Properties to apply to all edges (excluding 'id', 'from', 'to'). + verbosity: Logging verbosity level. At level >= 2, logs debug messages for + non-JSON-serializable attributes. + + Returns: + A tuple of (nodes_data, edges_data) lists suitable for JSON serialization. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/graph_utils.py` at line 55, Update the docstring for the helper that "converts a single NetworkX graph to JSON-serializable lists" to include a clear description of the verbosity parameter: name it "verbosity", list allowed values (e.g., 0, 1, 2 or False/True), state the default, and explain the behavior at each level (for example, verbosity==0: no debug logs; verbosity>=1: emit debug/info messages; verbosity>=2: emit verbose debug output and extra diagnostic details). Mention the effect on logging and any returned/side-effect behavior so callers know what to expect.temoa/utilities/unit_cost_explorer.py (1)
176-183: 🧹 Nitpick | 🔵 TrivialRename
mulitpliertomultiplierfor clarity.This typo is easy to propagate and makes the intent harder to scan.
Proposed refactor
-mulitplier: float = ( +multiplier: float = ( storage_dur * model.segment_fraction_per_season[2020, 'winter'] * model.days_per_period * c2a * c ) -print(f'The multiplier for the storage should be: {mulitplier}') +print(f'The multiplier for the storage should be: {multiplier}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/unit_cost_explorer.py` around lines 176 - 183, Rename the misspelled variable mulitplier to multiplier everywhere in this scope: change the assignment expression currently stored in mulitplier and the print call that references it so they use multiplier instead; ensure any other occurrences (e.g., later uses or return values in the same function/module) are updated to the correct name to avoid NameError or confusion. This affects the local variable defined from storage_dur * model.segment_fraction_per_season[2020, 'winter'] * model.days_per_period * c2a * c and the print(f'...{...}') line that reports its value.
♻️ Duplicate comments (2)
temoa/utilities/unit_cost_explorer.py (1)
163-172:⚠️ Potential issue | 🟠 MajorFix
TimeOfDayargument type in the constraint call.Line 169 passes
cast('TimeOfDay', '1')(string), butmodel.time_of_dayis integer-based (range(1, tod_slices + 1)). This can break key matching in indexed components.Proposed fix
upper_limit = storage_energy_upper_bound_constraint( model, cast('Region', 'A'), cast('Period', 2020), cast('Season', 'winter'), - cast('TimeOfDay', '1'), + cast('TimeOfDay', 1), cast('Technology', 'battery'), cast('Vintage', 2020), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/unit_cost_explorer.py` around lines 163 - 172, The TimeOfDay argument passed to storage_energy_upper_bound_constraint is cast as a string ('cast('TimeOfDay', '1')') which mismatches model.time_of_day (an integer range) and can break indexed lookups; change the TimeOfDay cast to an integer (e.g., cast('TimeOfDay', 1) or use an int-converted value) when calling storage_energy_upper_bound_constraint for Region 'A', Period 2020, Season 'winter', TimeOfDay 1, Technology 'battery', Vintage 2020 so the index keys align with model.time_of_day and preserves correct behavior of model.is_seasonal_storage and the constraint function.temoa/utilities/capacity_analyzer.py (1)
55-58:⚠️ Potential issue | 🟠 MajorGuard normalization against empty or zero-total capacity.
Line 57 can throw (
IndexError/ZeroDivisionError) whencapsis empty ortotal_cap == 0, which can happen after DB read errors or zero-only data.💡 Suggested fix
caps.sort() total_cap: float = sum(caps) +if not caps or total_cap == 0: + print('No capacity data to analyze') + raise SystemExit(0) cumulative_caps: list[float] = [ caps[0] / total_cap, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/capacity_analyzer.py` around lines 55 - 58, Guard the normalization against empty or zero-total capacity by checking caps and total_cap before dividing: compute total_cap = sum(caps) as before, then if not caps set cumulative_caps = [] (or return early), and if total_cap == 0 set cumulative_caps = [0.0 for _ in caps] (and optionally log a warning) instead of doing caps[0] / total_cap; ensure you update the code paths that use cumulative_caps accordingly (referencing total_cap, cumulative_caps, and caps in capacity_analyzer.py).
🤖 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/utilities/db_migration_v3_to_v3_1.py`:
- Around line 289-294: The current assignment stores a generator in
df_data["periods"] which gets exhausted on iteration; replace the inner
generator expression with an actual list so each row holds a reusable list of
periods (i.e., construct a list of p for p in time_optimize if v <= p < v +
lifetime_process[r, t, v]) when building df_data["periods"] so subsequent
membership checks against row["periods"] iterate reliably; keep references to
time_optimize and lifetime_process and the
df_data[["region","tech","vintage"]].values tuple (r, t, v) to locate and update
the comprehension.
---
Outside diff comments:
In `@temoa/utilities/graph_utils.py`:
- Line 55: Update the docstring for the helper that "converts a single NetworkX
graph to JSON-serializable lists" to include a clear description of the
verbosity parameter: name it "verbosity", list allowed values (e.g., 0, 1, 2 or
False/True), state the default, and explain the behavior at each level (for
example, verbosity==0: no debug logs; verbosity>=1: emit debug/info messages;
verbosity>=2: emit verbose debug output and extra diagnostic details). Mention
the effect on logging and any returned/side-effect behavior so callers know what
to expect.
In `@temoa/utilities/unit_cost_explorer.py`:
- Around line 176-183: Rename the misspelled variable mulitplier to multiplier
everywhere in this scope: change the assignment expression currently stored in
mulitplier and the print call that references it so they use multiplier instead;
ensure any other occurrences (e.g., later uses or return values in the same
function/module) are updated to the correct name to avoid NameError or
confusion. This affects the local variable defined from storage_dur *
model.segment_fraction_per_season[2020, 'winter'] * model.days_per_period * c2a
* c and the print(f'...{...}') line that reports its value.
---
Duplicate comments:
In `@temoa/utilities/capacity_analyzer.py`:
- Around line 55-58: Guard the normalization against empty or zero-total
capacity by checking caps and total_cap before dividing: compute total_cap =
sum(caps) as before, then if not caps set cumulative_caps = [] (or return
early), and if total_cap == 0 set cumulative_caps = [0.0 for _ in caps] (and
optionally log a warning) instead of doing caps[0] / total_cap; ensure you
update the code paths that use cumulative_caps accordingly (referencing
total_cap, cumulative_caps, and caps in capacity_analyzer.py).
In `@temoa/utilities/unit_cost_explorer.py`:
- Around line 163-172: The TimeOfDay argument passed to
storage_energy_upper_bound_constraint is cast as a string ('cast('TimeOfDay',
'1')') which mismatches model.time_of_day (an integer range) and can break
indexed lookups; change the TimeOfDay cast to an integer (e.g.,
cast('TimeOfDay', 1) or use an int-converted value) when calling
storage_energy_upper_bound_constraint for Region 'A', Period 2020, Season
'winter', TimeOfDay 1, Technology 'battery', Vintage 2020 so the index keys
align with model.time_of_day and preserves correct behavior of
model.is_seasonal_storage and the constraint function.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
stubs/pyomo/core/base/component.pyiis excluded by!stubs/**
📒 Files selected for processing (11)
pyproject.tomltemoa/utilities/capacity_analyzer.pytemoa/utilities/clear_db_outputs.pytemoa/utilities/database_util.pytemoa/utilities/db_migration_to_v3.pytemoa/utilities/db_migration_v3_1_to_v4.pytemoa/utilities/db_migration_v3_to_v3_1.pytemoa/utilities/graph_utils.pytemoa/utilities/run_all_v4_migrations.pytemoa/utilities/sql_migration_v3_1_to_v4.pytemoa/utilities/unit_cost_explorer.py
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
2 times, most recently
from
2ea2138 to
0e7b76f
Compare
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
4 times, most recently
from
4f1e42c to
8c5cf65
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/utilities/db_migration_v3_to_v3_1.py (1)
121-129:⚠️ Potential issue | 🟡 MinorDuplicate definition of
period_to_vintage_tables.The set
period_to_vintage_tablesis defined twice: first at lines 121-124, then again at lines 126-129 with identical content. Remove the duplicate definition.Proposed fix
period_to_vintage_tables = { "LimitNewCapacityShare", "LimitNewCapacity", } -period_to_vintage_tables = { - "LimitNewCapacityShare", - "LimitNewCapacity", -} - operator_added_tables: dict[str, tuple[str, str]] = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/db_migration_v3_to_v3_1.py` around lines 121 - 129, Remove the duplicate definition of the set named period_to_vintage_tables by keeping a single declaration and deleting the redundant one; locate both identical definitions of period_to_vintage_tables in the migration script (they appear consecutively) and leave only one occurrence so the symbol is defined once.
♻️ Duplicate comments (4)
temoa/utilities/capacity_analyzer.py (1)
26-33:⚠️ Potential issue | 🟠 MajorGuard empty/zero-capacity input before cumulative normalization.
When query/processing yields no capacity values, the current logic can crash on
caps[0] / total_cap.Suggested fix
except sqlite3.Error as e: print(e) + raise SystemExit(1) # chain them together into a list caps: list[float] = list(itertools.chain(*res)) @@ total_cap: float = sum(caps) +if not caps or total_cap == 0: + print('No capacity data to analyze') + raise SystemExit(0) cumulative_caps: list[float] = [ caps[0] / total_cap, ]Also applies to: 55-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/capacity_analyzer.py` around lines 26 - 33, The code can divide by zero or index an empty list when normalizing capacities (uses caps[0] / total_cap and cumulative normalization around lines referencing caps and total_cap); fix by checking if the query returned any capacity values and that total_cap > 0 before performing normalization: if caps is empty, return early (or set normalized list to zeros) and if total_cap == 0 skip division and handle normalization safely. Update the blocks that read DB results (the cursor loop that appends into res) and the normalization code that uses caps and total_cap to include these guards.temoa/utilities/database_util.py (1)
133-133:⚠️ Potential issue | 🟠 MajorUse concrete type expressions in
cast(...)targets.Quoted targets in
castshould be replaced with actual type expressions (str,tuple[str, str]) for cleaner, checker-friendly typing.Suggested fix
- return {cast('str', row[0]) for row in self.cur.execute(query)} + return {cast(str, row[0]) for row in self.cur.execute(query)} @@ - return {cast('str', row[0]) for row in self.cur.execute(query)} + return {cast(str, row[0]) for row in self.cur.execute(query)} @@ - return {cast('tuple[str, str]', row) for row in self.cur.execute(query)} + return {cast(tuple[str, str], row) for row in self.cur.execute(query)}#!/bin/bash # Verify no quoted cast targets remain in this module rg -nP "cast\\('\\w|cast\\('tuple\\[" temoa/utilities/database_util.py # Expected: no matchesAlso applies to: 179-179, 195-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/database_util.py` at line 133, Replace string-literal type targets in cast(...) with actual type expressions: change cast('str', row[0]) to cast(str, row[0]) and any cast('tuple[str, str]', ...) (or similar quoted forms) to cast(tuple[str, str], ...) where they appear (e.g., the return set comprehension using self.cur.execute(query) and the other occurrences mentioned around the cast calls on lines ~179 and ~195). Update all cast calls in temoa/utilities/database_util.py to use concrete types (str, tuple[str,str], etc.) so static checkers accept them.temoa/utilities/db_migration_v3_to_v3_1.py (1)
309-311: 🧹 Nitpick | 🔵 TrivialConsider using
df_data.emptyfor DataFrame emptiness check.Using
len(df_data) == 0works butdf_data.emptyis more idiomatic for pandas DataFrames.Proposed fix
- if len(df_data) == 0: + if df_data.empty: print("No data for: " + old_name) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/db_migration_v3_to_v3_1.py` around lines 309 - 311, Replace the len(df_data) == 0 emptiness check with the pandas-idiomatic df_data.empty; specifically, in the block where df_data and old_name are used (the conditional that currently reads if len(df_data) == 0: print("No data for: " + old_name) continue), change to use if df_data.empty: to test DataFrame emptiness while keeping the same print and continue behavior.temoa/utilities/unit_cost_explorer.py (1)
166-175:⚠️ Potential issue | 🟡 MinorType mismatch:
TimeOfDayshould be an integer, not a string.On line 128,
time_of_dayis constructed withrange(1, tod_slices + 1)which produces integers. However, line 172 casts'1'(a string) toTimeOfDay. This inconsistency may cause indexing issues when the constraint function looks up values in model sets.Proposed fix
upper_limit = storage_energy_upper_bound_constraint( model, cast('Region', 'A'), cast('Period', 2020), cast('Season', 'winter'), - cast('TimeOfDay', '1'), + cast('TimeOfDay', 1), cast('Technology', 'battery'), cast('Vintage', 2020), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/unit_cost_explorer.py` around lines 166 - 175, The call to storage_energy_upper_bound_constraint uses cast('TimeOfDay', '1') (a string) but the model's time_of_day set is populated with integers (range(1, tod_slices+1)); change the TimeOfDay argument to an integer (e.g., cast('TimeOfDay', 1) or use the same numeric variable from tod_slices/time_of_day) so storage_energy_upper_bound_constraint, model.is_seasonal_storage and any indexing using time_of_day match types.
🤖 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/utilities/db_migration_v3_to_v3_1.py`:
- Around line 199-214: The migration assumes every table in
period_to_vintage_tables has a vintage column but LimitNewCapacity and
LimitNewCapacityShare in v3.1 lack it; update the block that uses
new_cols.index("vintage") to first check whether "vintage" is present (e.g., if
"vintage" in new_cols) before computing vintage_index and transforming
data_transformed for the table named by new_name, and if not present simply skip
the period->vintage reshuffle for that table (or alternatively remove those
table names from period_to_vintage_tables); reference new_name,
period_to_vintage_tables, new_cols, vintage_index, and data_transformed to
locate and apply the guard.
---
Outside diff comments:
In `@temoa/utilities/db_migration_v3_to_v3_1.py`:
- Around line 121-129: Remove the duplicate definition of the set named
period_to_vintage_tables by keeping a single declaration and deleting the
redundant one; locate both identical definitions of period_to_vintage_tables in
the migration script (they appear consecutively) and leave only one occurrence
so the symbol is defined once.
---
Duplicate comments:
In `@temoa/utilities/capacity_analyzer.py`:
- Around line 26-33: The code can divide by zero or index an empty list when
normalizing capacities (uses caps[0] / total_cap and cumulative normalization
around lines referencing caps and total_cap); fix by checking if the query
returned any capacity values and that total_cap > 0 before performing
normalization: if caps is empty, return early (or set normalized list to zeros)
and if total_cap == 0 skip division and handle normalization safely. Update the
blocks that read DB results (the cursor loop that appends into res) and the
normalization code that uses caps and total_cap to include these guards.
In `@temoa/utilities/database_util.py`:
- Line 133: Replace string-literal type targets in cast(...) with actual type
expressions: change cast('str', row[0]) to cast(str, row[0]) and any
cast('tuple[str, str]', ...) (or similar quoted forms) to cast(tuple[str, str],
...) where they appear (e.g., the return set comprehension using
self.cur.execute(query) and the other occurrences mentioned around the cast
calls on lines ~179 and ~195). Update all cast calls in
temoa/utilities/database_util.py to use concrete types (str, tuple[str,str],
etc.) so static checkers accept them.
In `@temoa/utilities/db_migration_v3_to_v3_1.py`:
- Around line 309-311: Replace the len(df_data) == 0 emptiness check with the
pandas-idiomatic df_data.empty; specifically, in the block where df_data and
old_name are used (the conditional that currently reads if len(df_data) == 0:
print("No data for: " + old_name) continue), change to use if df_data.empty: to
test DataFrame emptiness while keeping the same print and continue behavior.
In `@temoa/utilities/unit_cost_explorer.py`:
- Around line 166-175: The call to storage_energy_upper_bound_constraint uses
cast('TimeOfDay', '1') (a string) but the model's time_of_day set is populated
with integers (range(1, tod_slices+1)); change the TimeOfDay argument to an
integer (e.g., cast('TimeOfDay', 1) or use the same numeric variable from
tod_slices/time_of_day) so storage_energy_upper_bound_constraint,
model.is_seasonal_storage and any indexing using time_of_day match types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac749a93-30c9-4a0e-ad76-2cb735918e83
⛔ Files ignored due to path filters (1)
stubs/pyomo/core/base/component.pyiis excluded by!stubs/**
📒 Files selected for processing (11)
pyproject.tomltemoa/utilities/capacity_analyzer.pytemoa/utilities/clear_db_outputs.pytemoa/utilities/database_util.pytemoa/utilities/db_migration_to_v3.pytemoa/utilities/db_migration_v3_1_to_v4.pytemoa/utilities/db_migration_v3_to_v3_1.pytemoa/utilities/graph_utils.pytemoa/utilities/run_all_v4_migrations.pytemoa/utilities/sql_migration_v3_1_to_v4.pytemoa/utilities/unit_cost_explorer.py
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
from
8c5cf65 to
8b256c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/utilities/unit_cost_explorer.py (1)
179-186:⚠️ Potential issue | 🟡 MinorTypo in variable name:
mulitpliershould bemultiplier.♻️ Suggested fix
-mulitplier: float = ( +multiplier: float = ( storage_dur * model.segment_fraction_per_season['winter'] * model.days_per_period * c2a * c ) -print(f'The multiplier for the storage should be: {mulitplier}') +print(f'The multiplier for the storage should be: {multiplier}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/unit_cost_explorer.py` around lines 179 - 186, The variable name mulitplier in temoa.utilities.unit_cost_explorer.py is misspelled; rename it to multiplier everywhere in the block (and throughout the function/module if referenced elsewhere) so the assignment (currently using storage_dur, model.segment_fraction_per_season['winter'], model.days_per_period, c2a, c) is stored in multiplier and update the print statement f'The multiplier for the storage should be: {mulitplier}' to use {multiplier}; also search for any other usages of mulitplier in functions/methods in this file and correct them to multiplier to avoid NameError.
♻️ Duplicate comments (3)
temoa/utilities/capacity_analyzer.py (1)
55-58:⚠️ Potential issue | 🟡 MinorPotential division by zero if
capsis empty.If
capsis empty after filtering,total_capwill be 0, causing aZeroDivisionErroron line 57 when computingcaps[0] / total_cap.🐛 Suggested guard
caps.sort() total_cap: float = sum(caps) +if not caps or total_cap == 0: + print('No capacity data to analyze') + sys.exit(0) cumulative_caps: list[float] = [ caps[0] / total_cap, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/capacity_analyzer.py` around lines 55 - 58, The code computes total_cap = sum(caps) and then uses caps[0] / total_cap to build cumulative_caps which can raise ZeroDivisionError or IndexError when caps is empty or sums to 0; update the logic in capacity_analyzer.py (around total_cap, cumulative_caps, caps) to guard against empty or all-zero caps by early-returning an empty cumulative_caps (or appropriate default), or check if total_cap == 0 before dividing and handle that case (e.g., normalize safely or skip division) so no division by zero or out-of-range access occurs.temoa/utilities/unit_cost_explorer.py (1)
166-175:⚠️ Potential issue | 🟡 MinorType mismatch:
TimeOfDayshould be an integer, not a string.On line 128,
time_of_dayis constructed withrange(1, tod_slices + 1)which produces integers. However, line 172 usescast('TimeOfDay', '1')(a string). This inconsistency may cause issues when the constraint function indexes into model sets.🐛 Proposed fix
upper_limit = storage_energy_upper_bound_constraint( model, cast('Region', 'A'), cast('Period', 2020), cast('Season', 'winter'), - cast('TimeOfDay', '1'), + cast('TimeOfDay', 1), cast('Technology', 'battery'), cast('Vintage', 2020), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/unit_cost_explorer.py` around lines 166 - 175, The call to storage_energy_upper_bound_constraint uses cast('TimeOfDay', '1') as a string which mismatches the integer TimeOfDay set (constructed via range in time_of_day); change the cast for the TimeOfDay argument to use an integer (e.g., cast('TimeOfDay', 1) or the appropriate integer variable) so it matches model.is_seasonal_storage and the storage_energy_upper_bound_constraint indexing; locate the call site around model.is_seasonal_storage[...] and update the TimeOfDay cast accordingly.temoa/utilities/database_util.py (1)
133-133: 🧹 Nitpick | 🔵 TrivialConsider removing quotes around type in
cast.Since the project requires Python 3.12+, you can simplify
cast('str', row[0])tocast(str, row[0])without the string literal. The same applies to lines 179 and 195.♻️ Suggested simplification
- return {cast('str', row[0]) for row in self.cur.execute(query)} + return {cast(str, row[0]) for row in self.cur.execute(query)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/database_util.py` at line 133, Replace string-literal type hints in cast calls with actual types: change occurrences of cast('str', row[0]) to cast(str, row[0]) (and similarly for the other two occurrences on the same file) so the cast uses the built-in str type rather than a quoted name; search for the exact expression "cast('str', row[0])" in temoa/utilities/database_util.py (also at the other two spots mentioned) and update them to cast(str, row[0]).
🤖 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/utilities/capacity_analyzer.py`:
- Around line 19-20: The hardcoded source_db_file = 'US_9R_8D_CT500.sqlite'
should be made configurable and validated: replace the literal with a resolved
value obtained from a CLI argument (use argparse) or environment variable (e.g.,
SOURCE_DB_FILE) with the current hardcoded name as an optional default; call
os.path.abspath on the provided value and verify existence with os.path.exists,
raising or logging a clear error if the file is missing. Update any code that
references source_db_file to use the new variable and ensure functions that open
the DB receive the resolved path.
In `@temoa/utilities/db_migration_v3_1_to_v4.py`:
- Around line 135-200: migrate_all is doing many special-case branches; extract
the special-case logic into small helper functions (e.g.,
_migrate_capacity_factor_process(con_old, con_new, old) to contain the
CapacityFactorProcess aggregation/insert and return count, and
_migrate_time_tables(con_old, con_new, old) to encapsulate all time/table-name
skips and mappings) so migrate_all becomes a simple loop that delegates: call
_migrate_capacity_factor_process when old == 'CapacityFactorProcess', call
_migrate_time_tables for the various time/meta tables (or have it return a
boolean to skip), otherwise compute new = map_token_no_cascade(old) and use
migrate_direct_table(con_old, con_new, old, new); ensure each helper returns
number of rows migrated (or 0) and raises errors as needed so migrate_all can
accumulate total and keep the existing try/except behavior.
- Around line 39-41: The list comprehension creating CUSTOM_KEYS_SORTED should
iterate directly over CUSTOM_MAP instead of calling .keys(); update the
expression to use [k for k in CUSTOM_MAP if k not in CUSTOM_EXACT_ONLY] (keeping
the sorted(..., key=lambda k: -len(k)) wrapper and the same symbols
CUSTOM_KEYS_SORTED, CUSTOM_MAP, and CUSTOM_EXACT_ONLY).
- Around line 259-260: The try/except around the migration of
time_season_sequential currently swallows sqlite3.OperationalError and TypeError
with a bare pass; replace the pass with the same warning behavior used in the
other migration blocks (e.g., the prints at the other except handlers) so
failures are visible—catch (sqlite3.OperationalError, TypeError) and log/print a
descriptive warning that includes the name time_season_sequential and the
exception details (e.g., str(e)) to aid debugging.
- Around line 28-29: CUSTOM_MAP contains inconsistent key casing
('CommodityDStreamProcess' vs 'commodityUStreamProcess') that likely doesn't
match the real v3.1 source table names; inspect the v3.1 schema for the actual
table identifiers and either (A) correct the keys in CUSTOM_MAP to exactly match
the source table names (preserve capitalization) mapping to the intended
snake_case targets, or (B) remove the mappings if those tables do not exist in
v3.1, then run the migration/unit tests to confirm no dead entries remain; focus
your changes on the CUSTOM_MAP dictionary and the specific keys
'CommodityDStreamProcess' and 'commodityUStreamProcess'.
In `@temoa/utilities/db_migration_v3_to_v3_1.py`:
- Around line 32-41: The module currently runs parser.parse_args() and opens
sqlite3 connections (creating legacy_db, schema_file, new_db_name, new_db_path,
con_old, con_new, cur) at import time; move that logic into a main() function
(e.g., create a function named main that performs parser.parse_args(), computes
legacy_db/new_db_path, and calls sqlite3.connect to produce con_old and con_new)
and call main() under if __name__ == "__main__": so importing the module doesn’t
execute migration setup and connections.
- Around line 121-129: Duplicate definition of the set period_to_vintage_tables
appears twice; remove the redundant second (or first) definition so the variable
is defined only once. Locate the repeated period_to_vintage_tables assignment
and delete the duplicate block, leaving a single set declaration (retaining the
entries "LimitNewCapacityShare" and "LimitNewCapacity"); ensure no other code
relies on the duplicated line and run tests to confirm behavior unchanged.
- Around line 264-286: The type annotations for time_all and time_optimize are
incorrect: they are declared as list[str] but TimePeriod.period is INTEGER;
change both annotations to list[int] (i.e., time_all: list[int] and
time_optimize: list[int]) and ensure any code that builds or uses these lists
(the comprehensions that populate them, the sorted() calls, and their usage when
constructing keys for lifetime_process or iterating vintages) treats them as
ints (no string assumptions). Update references around the lifetime_process
construction (where keys use vintages) to match the int-typed periods and keep
TemoaModel.default_lifetime_tech and existing dict key shapes unchanged.
- Around line 417-421: The print message incorrectly says "LifetimeLoanTech to
LifetimeLoanProcess" and should be "LoanLifetimeTech to LoanLifetimeProcess";
update the f-string accordingly. Also replace the loop that appends multiple
rows to new_data (the code iterating over vints and calling
new_data.append((row[0], row[1], v, row[2], row[3]))) with a single extend of a
list comprehension to build all tuples at once (use new_data.extend([...]) or
assign new_data += [...] ) to improve performance before calling
con_new.executemany(query, new_data).
In `@temoa/utilities/sql_migration_v3_1_to_v4.py`:
- Around line 46-49: The comprehension in CUSTOM_KEYS_SORTED uses the redundant
.keys() call on CUSTOM_MAP; change the filter to iterate/check membership
directly against CUSTOM_MAP (i.e., use "k for k in CUSTOM_MAP if k not in
CUSTOM_EXACT_ONLY") so CUSTOM_KEYS_SORTED still sorts by -len(k) but avoids the
unnecessary .keys() allocation; update the expression that builds
CUSTOM_KEYS_SORTED and leave CUSTOM_EXACT_ONLY and CUSTOM_MAP unchanged.
- Around line 354-355: The print message after inserting sequential seasons is
misleading — it says "Propagated {len(old_data)} sequential seasons to
time_season" while the code actually inserts into time_season_sequential; update
the log to reference the correct table (time_season_sequential) and keep the
variable reference (len(old_data)) so the output accurately reflects the target
table and row count where the insertion occurs.
- Around line 136-138: The except block that currently reads "except Exception
as e:" should be narrowed to only the expected errors when loading the v3.1 dump
(e.g., catch sqlite3.Error for DB errors and OSError/IOError for file I/O) so
you don't swallow KeyboardInterrupt/SystemExit; update the handler in the
function containing the dump loading logic to catch those specific exceptions,
import sqlite3 if not already present, and keep the same error message/exit
behavior (print the error and sys.exit(1)) for those cases only.
- Around line 149-151: The handler currently uses a bare "except Exception as e"
when applying the v4 schema; replace it with catching specific expected
exception types from the DB/IO modules used (for example sqlite3.Error,
psycopg2.Error, or IOError/OperationalError as appropriate) or a tuple of those
exceptions instead of Exception, then keep the existing print(f'ERROR: Failed to
apply v4 schema: {e}') and sys.exit(1) behavior; locate the except block that
matches "except Exception as e" (the v4 schema apply/commit block) and update it
to "except (sqlite3.Error, SomeOtherError) as e:" or the concrete exceptions
used by your DB client.
---
Outside diff comments:
In `@temoa/utilities/unit_cost_explorer.py`:
- Around line 179-186: The variable name mulitplier in
temoa.utilities.unit_cost_explorer.py is misspelled; rename it to multiplier
everywhere in the block (and throughout the function/module if referenced
elsewhere) so the assignment (currently using storage_dur,
model.segment_fraction_per_season['winter'], model.days_per_period, c2a, c) is
stored in multiplier and update the print statement f'The multiplier for the
storage should be: {mulitplier}' to use {multiplier}; also search for any other
usages of mulitplier in functions/methods in this file and correct them to
multiplier to avoid NameError.
---
Duplicate comments:
In `@temoa/utilities/capacity_analyzer.py`:
- Around line 55-58: The code computes total_cap = sum(caps) and then uses
caps[0] / total_cap to build cumulative_caps which can raise ZeroDivisionError
or IndexError when caps is empty or sums to 0; update the logic in
capacity_analyzer.py (around total_cap, cumulative_caps, caps) to guard against
empty or all-zero caps by early-returning an empty cumulative_caps (or
appropriate default), or check if total_cap == 0 before dividing and handle that
case (e.g., normalize safely or skip division) so no division by zero or
out-of-range access occurs.
In `@temoa/utilities/database_util.py`:
- Line 133: Replace string-literal type hints in cast calls with actual types:
change occurrences of cast('str', row[0]) to cast(str, row[0]) (and similarly
for the other two occurrences on the same file) so the cast uses the built-in
str type rather than a quoted name; search for the exact expression "cast('str',
row[0])" in temoa/utilities/database_util.py (also at the other two spots
mentioned) and update them to cast(str, row[0]).
In `@temoa/utilities/unit_cost_explorer.py`:
- Around line 166-175: The call to storage_energy_upper_bound_constraint uses
cast('TimeOfDay', '1') as a string which mismatches the integer TimeOfDay set
(constructed via range in time_of_day); change the cast for the TimeOfDay
argument to use an integer (e.g., cast('TimeOfDay', 1) or the appropriate
integer variable) so it matches model.is_seasonal_storage and the
storage_energy_upper_bound_constraint indexing; locate the call site around
model.is_seasonal_storage[...] and update the TimeOfDay cast accordingly.
🪄 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: d4ebee5d-e313-49cd-b6d9-aeee445ba40a
⛔ Files ignored due to path filters (1)
stubs/pyomo/core/base/component.pyiis excluded by!stubs/**
📒 Files selected for processing (10)
pyproject.tomltemoa/utilities/capacity_analyzer.pytemoa/utilities/clear_db_outputs.pytemoa/utilities/database_util.pytemoa/utilities/db_migration_to_v3.pytemoa/utilities/db_migration_v3_1_to_v4.pytemoa/utilities/db_migration_v3_to_v3_1.pytemoa/utilities/graph_utils.pytemoa/utilities/sql_migration_v3_1_to_v4.pytemoa/utilities/unit_cost_explorer.py
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
from
cbcec2d to
d6a08ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
temoa/utilities/db_migration_v3_to_v3_1.py (1)
198-203:⚠️ Potential issue | 🔴 CriticalGuard the period→vintage reshuffle on tables that actually have
vintage.This still does
new_cols.index("vintage")unconditionally for every table inperiod_to_vintage_tables. When one of those destination tables only hasperiod, the migration aborts here withValueError.Suggested fix
- if new_name in period_to_vintage_tables: + if new_name in period_to_vintage_tables and "vintage" in new_cols: old_cols: list[str] = [ c[1] for c in con_old.execute(f"PRAGMA table_info({old_name});").fetchall() ] period_index = old_cols.index("period") vintage_index = new_cols.index("vintage")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/utilities/db_migration_v3_to_v3_1.py` around lines 198 - 203, The code assumes every destination table in period_to_vintage_tables has a "vintage" column and calls new_cols.index("vintage") unconditionally, which raises ValueError when "vintage" is missing; modify the block that handles new_name in period_to_vintage_tables to first check if "vintage" is in new_cols (e.g., if "vintage" not in new_cols: continue or skip reshuffle for this table) before computing vintage_index, so only tables that actually have a vintage column perform the period→vintage reshuffle; keep existing variables (new_name, old_name, old_cols, new_cols, period_index, vintage_index, con_old.execute) to locate where to add the guard.
🤖 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/utilities/capacity_analyzer.py`:
- Line 12: The import of typing.Any weakens type safety—replace Any with a
concrete row type: define a TypedDict or dataclass (e.g., QueryRow or
CapacityRow) that models the query/result fields used in this module, then
change res: list[Any] to res: list[QueryRow] and update any function signatures
in capacity_analyzer (e.g., functions that build/return res) to return
List[QueryRow]; also update other occurrences noted (the other res declarations
and return annotations) to use the new TypedDict/dataclass so mypy/static checks
can validate field access.
- Around line 80-86: The cumulative cutoff logic skips evaluating the first
source because you only update cutoff_num_sources inside the loop over caps[1:],
so if the first cumulative contribution (cumulative_caps[0] computed from
caps[0] / total_cap) is below 5% it never increments; update the code to check
the initial cumulative value before the loop (or initialize cutoff_num_sources
based on cumulative_caps[0]), then continue the existing loop over caps[1:]
updating cumulative_caps and incrementing cutoff_num_sources when
cumulative_caps[-1] < 0.05; reference cumulative_caps, caps, total_cap, and
cutoff_num_sources when making this change.
- Around line 53-54: The except block in capacity_analyzer.py that currently
does "except sqlite3.Error as e: print(e)" should fail fast instead of silently
continuing: replace the print with a logged error (e.g., logger.exception or
process_logger.error) and then either re-raise the exception or exit with a
non-zero status (e.g., raise or sys.exit(1)) so the script does not continue
with partial data; keep the reference to sqlite3.Error in the handler and update
any surrounding function (the DB read/loader function in capacity_analyzer.py)
to propagate the failure.
In `@temoa/utilities/db_migration_v3_1_to_v4.py`:
- Around line 213-215: The code uses truthiness to decide whether to run the
sequential-season migration by checking "if first_period:" after executing
con_old.execute('SELECT MIN(period) FROM TimeSeasonSequential').fetchone() and
assigning first_period = res[0]; change this to explicitly test for None (e.g.,
"if first_period is not None") so that a legitimate period value of 0 is treated
as real data and the migration runs; update the condition surrounding the
TimeSeasonSequential migration logic that references first_period accordingly.
- Around line 236-243: The script currently connects to the target v4 DB (out)
before verifying it doesn't already exist, which can silently reuse/stale data;
modify the flow to check Path(args.out) (out) existence before opening/creating
the DB and abort if it exists (raise a FileExistsError or exit non-zero with a
clear message), i.e., perform the existence check on out prior to calling
sqlite3.connect(out) and only create/connect when confirmed absent; keep using
src, schema, con_old, con_new and con_new.executescript(sql) but ensure the
pre-check prevents accidental reuse of an existing v4 file.
In `@temoa/utilities/db_migration_v3_to_v3_1.py`:
- Around line 152-157: The code currently opens the target DB with
sqlite3.connect(new_db_path) which will reuse an existing file; add an explicit
existence check on new_db_path (using Path.exists()) before creating con_new and
refuse to proceed if the file already exists (raise an exception or log error
and exit) so the migration never reuses an existing target database; update the
code around new_db_name/new_db_path and the call to sqlite3.connect(new_db_path)
to perform this check and stop execution if the target file exists.
In `@temoa/utilities/sql_migration_v3_1_to_v4.py`:
- Around line 339-357: The MIN(period) check currently treats period=0 as falsy
and silently swallows errors; change the condition to "first_period is not None"
when reading first_period from con_old_in_memory so period=0 is preserved, and
replace the bare "except sqlite3.OperationalError: pass" with the same
warning/logging behavior used by the other migration blocks (emit a warning that
time_season_sequential migration failed) so failures are visible; keep the
existing SELECT/INSERT logic using con_old_in_memory and con_new_in_memory and
the time_season_sequential / seas_seq, season, segment_fraction columns
unchanged.
---
Duplicate comments:
In `@temoa/utilities/db_migration_v3_to_v3_1.py`:
- Around line 198-203: The code assumes every destination table in
period_to_vintage_tables has a "vintage" column and calls
new_cols.index("vintage") unconditionally, which raises ValueError when
"vintage" is missing; modify the block that handles new_name in
period_to_vintage_tables to first check if "vintage" is in new_cols (e.g., if
"vintage" not in new_cols: continue or skip reshuffle for this table) before
computing vintage_index, so only tables that actually have a vintage column
perform the period→vintage reshuffle; keep existing variables (new_name,
old_name, old_cols, new_cols, period_index, vintage_index, con_old.execute) to
locate where to add the guard.
🪄 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: 891963da-e7a5-401f-bbc1-1647973b2f5b
📒 Files selected for processing (6)
temoa/utilities/capacity_analyzer.pytemoa/utilities/database_util.pytemoa/utilities/db_migration_v3_1_to_v4.pytemoa/utilities/db_migration_v3_to_v3_1.pytemoa/utilities/sql_migration_v3_1_to_v4.pytemoa/utilities/unit_cost_explorer.py
ParticularlyPythonicBS
added
Maintenance
ParticularlyPythonicBS
force-pushed
the
typing/utilities
branch
from
d6a08ad to
d5d57ec
Compare
1 participant
adds typing to the utilities sub dir
Summary by CodeRabbit