adding types to utilities #252
Conversation
WalkthroughNarrowed mypy exclusions to include Changes
Sequence Diagram(s)(omitted — changes are typing/guards and small signature tweaks, not new multi-component control flows) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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).
| model.v_storage_level.construct() | ||
| model.segment_fraction_per_season.construct() |
There was a problem hiding this comment.
Duplicate construct() calls detected.
model.v_storage_level.construct() and model.segment_fraction_per_season.construct() are already called on lines 138-139. These duplicate calls are likely unintentional and should be removed.
Proposed fix
-model.v_storage_level.construct()
-model.segment_fraction_per_season.construct()
-
cast('Any', model.is_seasonal_storage)['battery'] = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| model.v_storage_level.construct() | |
| model.segment_fraction_per_season.construct() | |
| cast('Any', model.is_seasonal_storage)['battery'] = False |
🤖 Prompt for AI Agents
In @temoa/utilities/unit_cost_explorer.py around lines 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.
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.
temoa/utilities/capacity_analyzer.py
Outdated
| # Attempt to locate data_files within the installed package or nearby | ||
| try: | ||
| # Python 3.9+ preferred method | ||
| from importlib.resources import files | ||
|
|
||
| # Assuming data_files is at the root of the 'temoa' package or repo structure | ||
| # If installed, 'temoa' package might contain data_files or it might be in share | ||
| # For now, let's try to find it relative to 'temoa' package prompt | ||
| # If 'temoa' is the top level package: | ||
| qt = files('temoa') | ||
| source_db_file: str = str(qt.joinpath('data_files', 'untracked_data', db)) | ||
| except (ImportError, AttributeError): | ||
| # Fallback for older python or if files() is not working as expected | ||
| # This part is tricky without knowing exact install layout. | ||
| # Assuming dev install where this script is in temoa/temoa/utilities | ||
| # and data_files is in temoa/data_files | ||
| current_file = Path(__file__).resolve() | ||
| # temoa/temoa/utilities/capacity_analyzer.py -> temoa/data_files | ||
| source_db_file = str(current_file.parents[2] / 'data_files' / 'untracked_data' / db) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the capacity_analyzer.py file and show its full path
fd -t f "capacity_analyzer.py" --full-path
# Show the directory structure for temoa
fd -t d "temoa" -x ls -la {} \; 2>/dev/null | head -20Repository: TemoaProject/temoa
Length of output: 1201
🏁 Script executed:
# Find data_files directory
fd -t d "data_files" --full-path
# Show structure to understand where data_files should be
ls -la | head -20
fd -t d "untracked_data" --full-pathRepository: TemoaProject/temoa
Length of output: 1374
Fix incorrect path comment in fallback fallback logic.
The comment on line 40 is misleading. The actual file path is temoa/utilities/capacity_analyzer.py (not temoa/temoa/utilities/...), and data_files is located at the repository root, not inside temoa/. The .parents[2] calculation is correct and properly reaches the repository root to access data_files/, but the comment should be updated to reflect the actual directory structure: # temoa/utilities/capacity_analyzer.py -> data_files (at repo root).
🤖 Prompt for AI Agents
In @temoa/utilities/capacity_analyzer.py around lines 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.
| query = f'SELECT tech FROM Technology WHERE flag IN ({in_clause})' | ||
|
|
||
| return {row[0] for row in self.cur.execute(query)} | ||
| return {cast('str', row[0]) for row in self.cur.execute(query)} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM, but consider removing quotes around type.
The cast is correct for type narrowing. Since the project requires Python 3.12+, you can simplify to cast(str, row[0]) without the string literal.
🤖 Prompt for AI Agents
In @temoa/utilities/database_util.py at 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.
| lifetime_process: dict[tuple[Any, ...], float] = {} | ||
| data_rows = cur.execute("SELECT region, tech, vintage FROM Efficiency").fetchall() | ||
| for rtv in data_rows: | ||
| lifetime_process[rtv] = TemoaModel.default_lifetime_tech | ||
| data = cur.execute("SELECT region, tech, lifetime FROM LifetimeTech").fetchall() | ||
| for rtl in data: | ||
| data_rows = cur.execute("SELECT region, tech, lifetime FROM LifetimeTech").fetchall() | ||
| for rtl in data_rows: | ||
| for v in time_all: | ||
| lifetime_process[*rtl[0:2], v] = rtl[2] | ||
| data = cur.execute("SELECT region, tech, vintage, lifetime FROM LifetimeProcess").fetchall() | ||
| for rtvl in data: | ||
| lifetime_process[(*rtl[0:2], v)] = rtl[2] | ||
| data_rows = cur.execute("SELECT region, tech, vintage, lifetime FROM LifetimeProcess").fetchall() | ||
| for rtvl in data_rows: | ||
| lifetime_process[rtvl[0:3]] = rtvl[3] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more specific tuple type.
dict[tuple[Any, ...], float] works but could be more precise. Since keys are (region, tech, vintage), consider:
lifetime_process: dict[tuple[str, str, int], float] = {}Or if vintage types vary, at least dict[tuple[str, str, Any], float].
🤖 Prompt for AI Agents
In @temoa/utilities/db_migration_v3_to_v3_1.py around lines 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.
| if len(df_data) == 0: | ||
| print("No data for: " + old_name) | ||
| continue |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer df_data.empty for DataFrame emptiness check.
Using len(df_data) == 0 works but df_data.empty is more idiomatic for pandas DataFrames.
- if len(df_data) == 0:
+ if df_data.empty:
print("No data for: " + old_name)
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(df_data) == 0: | |
| print("No data for: " + old_name) | |
| continue | |
| if df_data.empty: | |
| print("No data for: " + old_name) | |
| continue |
🤖 Prompt for AI Agents
In @temoa/utilities/db_migration_v3_to_v3_1.py around lines 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.
| rtv: tuple[str, str, int] = ('A', 'battery', 2020) # rtv | ||
| rptv: tuple[str, int, str, int] = ('A', 2020, 'battery', 2020) # rptv |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using type alias syntax for clarity.
Since this project requires Python 3.12+, you could use PEP 695 type alias syntax for these recurring index types:
type RTV = tuple[str, str, int]
type RPTV = tuple[str, int, str, int]This would make the intent clearer and allow reuse elsewhere. Based on learnings, PEP 695 type syntax is preferred for type aliases in this project.
🤖 Prompt for AI Agents
In @temoa/utilities/unit_cost_explorer.py around lines 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.
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.
| model.v_storage_level.construct() | ||
| model.segment_fraction_per_season.construct() | ||
|
|
There was a problem hiding this comment.
Duplicate construct() calls detected.
v_storage_level.construct() and segment_fraction_per_season.construct() are called twice - once at lines 142-143 and again at lines 145-146. This appears to be an unintentional duplication that could cause issues if construct() is not idempotent.
🐛 Remove duplicate calls
-model.v_storage_level.construct()
-model.segment_fraction_per_season.construct()
-
model.is_seasonal_storage[cast('Technology', 'battery')] = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| model.v_storage_level.construct() | |
| model.segment_fraction_per_season.construct() | |
| model.is_seasonal_storage[cast('Technology', 'battery')] = False |
🤖 Prompt for AI Agents
In @temoa/utilities/unit_cost_explorer.py around lines 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.
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.
| total_cap: float = sum(caps) | ||
| cumulative_caps: list[float] = [ | ||
| caps[0] / total_cap, | ||
| ] |
There was a problem hiding this comment.
Potential division by zero if caps is empty.
If caps is empty after filtering, total_cap will be 0, causing a ZeroDivisionError on line 57.
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
In @temoa/utilities/capacity_analyzer.py around lines 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.
| model.is_seasonal_storage[cast('Technology', 'battery')] = False | ||
| upper_limit = storage_energy_upper_bound_constraint( | ||
| model, | ||
| cast('Region', 'A'), | ||
| cast('Period', 2020), | ||
| cast('Season', 'winter'), | ||
| cast('TimeOfDay', '1'), | ||
| cast('Technology', 'battery'), | ||
| cast('Vintage', 2020), | ||
| ) |
There was a problem hiding this comment.
Potential type mismatch: TimeOfDay should be an integer, not a string.
On line 129, time_of_day is constructed with range(1, tod_slices + 1) which produces integers. However, line 169 casts '1' (a string) to TimeOfDay. 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),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| model.is_seasonal_storage[cast('Technology', 'battery')] = False | |
| upper_limit = storage_energy_upper_bound_constraint( | |
| model, | |
| cast('Region', 'A'), | |
| cast('Period', 2020), | |
| cast('Season', 'winter'), | |
| cast('TimeOfDay', '1'), | |
| cast('Technology', 'battery'), | |
| cast('Vintage', 2020), | |
| ) | |
| model.is_seasonal_storage[cast('Technology', 'battery')] = False | |
| upper_limit = storage_energy_upper_bound_constraint( | |
| model, | |
| cast('Region', 'A'), | |
| cast('Period', 2020), | |
| cast('Season', 'winter'), | |
| cast('TimeOfDay', 1), | |
| cast('Technology', 'battery'), | |
| cast('Vintage', 2020), | |
| ) |
🤖 Prompt for AI Agents
In @temoa/utilities/unit_cost_explorer.py around lines 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'.
adds typing to the utilities sub dir
Summary by CodeRabbit
Type Safety & Quality
API Enhancements
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.