typing: adding types to extensions#251
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoved the broad mypy exclusion for extensions and applied widespread typing and robustness changes across many temoa/extensions modules (postponed annotations, TYPE_CHECKING guards, explicit type hints, safer DB access, minor control-flow/logging adjustments). Changes
Sequence Diagram(s)(omitted) 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🧪 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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
temoa/extensions/single_vector_mga/sv_mga_sequencer.py (1)
203-209: Theverboseparameter is declared but never used.Static analysis correctly flags that
verbose(line 208) is unused in the function body (ARG004). The docstring references it at line 215, but there's no actual implementation using it.Either remove the unused parameter or implement the intended verbose behavior (e.g., conditional print/log statements).
🔧 Option 1: Remove unused parameter
@staticmethod def construct_obj( model: TemoaModel, emission_labels: Iterable[str], capacity_labels: Iterable[str], activity_labels: Iterable[str], - verbose: bool = True, ) -> Expression | int: """ Construct an alternative OBJ statement from the config data Specifically, locate the labels passed in within the related variables and kluge together an objective to be minimized from them. - :param verbose: If True, report to console during construction... :param M: The basis model to search🔧 Option 2: Use the parameter (if verbose output is intended)
# handle emissions... for label in emission_labels: idxs = [idx for idx in model.emission_activity if idx[1] == label] logger.debug('Located %d items for emission label: %s', len(idxs), label) + if verbose: + print(f'Located {len(idxs)} items for emission label: {label}')Additionally, if you keep the parameter, consider making it keyword-only to address the boolean positional argument warnings (FBT001/FBT002):
- verbose: bool = True, + *, + verbose: bool = True,temoa/extensions/stochastics/scenario_creator.py (1)
21-33: Consider narrowing the return type toTemoaModel.The static analysis flags
Anyusage (ANN401). WhileAnyin**kwargsis acceptable given mpi-sppy's untyped callback interface, the return type could potentially beTemoaModelsincebuild_instanceexplicitly returns that type. However, sinceattach_root_nodemutates the instance with mpi-sppy-specific attributes, keepingAnyis also a defensible choice.♻️ Optional: Narrow return type
-def scenario_creator(scenario_name: str, **kwargs: Any) -> Any: +def scenario_creator(scenario_name: str, **kwargs: Any) -> TemoaModel:This would require adding
TemoaModelto the TYPE_CHECKING imports:if TYPE_CHECKING: from temoa.core.config import TemoaConfig from temoa.data_io.hybrid_loader import LoadItem from temoa.extensions.stochastics.stochastic_config import StochasticConfig from temoa.temoa_model.temoa_model import TemoaModeltemoa/extensions/myopic/myopic_progress_mapper.py (1)
52-83: Consider usingLiteralfor thestatusparameter.The function validates that
statusmust be one of{'load', 'solve', 'report', 'check'}. Using aLiteraltype would catch invalid values at static analysis time rather than runtime.♻️ Optional improvement
+from typing import Literal + +StatusType = Literal['load', 'solve', 'report', 'check'] + class MyopicProgressMapper: ... - def report(self, mi: MyopicIndex, status: str) -> None: + def report(self, mi: MyopicIndex, status: StatusType) -> None:temoa/extensions/get_comm_tech.py (1)
12-44: SQL injection vulnerability inget_tperiods.Line 35 interpolates
y(derived from database content) directly into the SQL query using an f-string. Whileyoriginates from prior query results (not direct user input), this pattern is fragile and could be exploited if the data source is compromised.🔒 Proposed fix using parameterized query
for y in x: cur.execute( - f"SELECT DISTINCT period FROM output_flow_out WHERE scenario is '{y}'" + 'SELECT DISTINCT period FROM output_flow_out WHERE scenario = ?', + (y,) )temoa/extensions/modeling_to_generate_alternatives/worker.py (1)
35-59: Add return type annotation to__init__.Per Ruff ANN204, the
__init__method should have a-> Nonereturn type annotation for completeness with the typing pass.Proposed fix
def __init__( self, model_queue: Queue[Any], results_queue: Queue[Any], log_root_name: str, log_queue: Queue[logging.LogRecord], log_level: int = logging.INFO, solver_name: str = 'appsi_highs', solver_options: dict[str, Any] | None = None, solver_log_path: Path | None = None, - ): + ) -> None:temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (1)
298-302: Fix incorrect string formatting in RuntimeError.
RuntimeErrordoesn't support%-style formatting in the constructor. Thevar_namewon't be interpolated into the message.Proposed fix
if not isinstance(var, Var): raise RuntimeError( - 'Failed to retrieve a named variable from the model: %s', var_name + f'Failed to retrieve a named variable from the model: {var_name}' )temoa/extensions/method_of_morris/morris.py (1)
40-49: Function relies on module-level globalsconfiganddb_file.The
evaluatefunction referencesconfig(line 42, 46) anddb_file(line 49) which are defined later in the module during script execution. This works because the function is only called after these globals are initialized, but it makes the function harder to test and reuse. Consider passing these as parameters, similar to howmorris_evaluate.pyhandles this.Note: The related
morris_evaluate.pyfile passesconfigas a parameter toevaluate, which is a cleaner pattern.temoa/extensions/monte_carlo/mc_worker.py (1)
46-67: Add return type annotation-> Nonefor__init__.Per static analysis hint (ANN204), special methods like
__init__should have explicit return type annotations for consistency with the typing effort in this PR.Suggested fix
def __init__( self, dp_queue: Queue[Any], results_queue: Queue[DataBrick | str], log_root_name: str, log_queue: Queue[logging.LogRecord], log_level: int = logging.INFO, solver_log_path: Path | None = None, **kwargs: Any, - ): + ) -> None:temoa/extensions/monte_carlo/mc_sequencer.py (1)
55-108: Add return type annotation-> Nonefor__init__.Per static analysis hint (ANN204),
__init__should have an explicit return type annotation.Suggested fix
- def __init__(self, config: TemoaConfig): + def __init__(self, config: TemoaConfig) -> None:temoa/extensions/monte_carlo/mc_run.py (1)
43-54: Add return type annotation-> Nonefor__init__methods.The
__init__methods forTweak,TweakFactory,MCRun, andMCRunFactoryclasses are all missing the-> Nonereturn type annotation. For consistency with the typing effort in this PR, consider adding them.Suggested fixes for all __init__ methods
# Tweak.__init__ (line 43) - def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float): + def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float) -> None: # TweakFactory.__init__ (line 70) - def __init__(self, data_store: dict[str, Any]): + def __init__(self, data_store: dict[str, Any]) -> None: # MCRun.__init__ (line 175) def __init__( self, scenario_name: str, run_index: int, data_store: dict[str, Any], included_tweaks: dict[Tweak, list[ChangeRecord]], - ): + ) -> None: # MCRunFactory.__init__ (line 224) - def __init__(self, config: TemoaConfig, data_store: dict[str, Any]): + def __init__(self, config: TemoaConfig, data_store: dict[str, Any]) -> None:
🤖 Fix all issues with AI agents
In @temoa/extensions/method_of_morris/morris_evaluate.py:
- Around line 65-67: The format string stored in setting_entry is never
interpolated before being appended to log_entry, so the log prints the raw
"%d/%s/%f" placeholders; update the code where setting_entry is appended to
instead append a formatted string (e.g., use setting_entry % (run_number,
param_name, param_index, param_value) or construct an f-string) and ensure the
variables used match the expected types, then call logger.debug on the joined
log_entry as before.
In @temoa/extensions/method_of_morris/morris_sequencer.py:
- Around line 88-96: The cast('Any', pert) followed by float() is unnecessary
and unconventional; update the assignment for self.mm_perturbation to
convert/match the expected numeric type directly (e.g., self.mm_perturbation =
float(pert) if you trust morris_inputs, or use self.mm_perturbation =
float(cast(float, pert)) to keep an explicit type cast) when reading pert from
morris_inputs.get('perturbation'), leaving the default fallback logic and logger
calls unchanged.
In @temoa/extensions/method_of_morris/morris.py:
- Around line 24-25: The evaluate function has an unused parameter k; rename it
to _k in the function signature (def evaluate(param_names: dict[int, list[Any]],
param_values: Any, data: dict[str, Any], _k: int) -> list[Any]) to follow Python
convention for intentionally unused parameters and update any internal
references to use _k if present.
- Line 55: SQL queries use the wrong table name `output_emissionn`; update the
SQL in the cur.execute calls to use the correct table `output_emission` (e.g.,
replace the query string in the cur.execute(...) call found in morris.py and the
analogous cur.execute in clear_db_outputs.py) so the database selects run
against the actual table name.
In @temoa/extensions/modeling_to_generate_alternatives/hull.py:
- Line 28: The __init__ method in class Hull declares an unused **kwargs
parameter; either remove it if not needed or rename it to **_kwargs to signal
intentional non-use and silence the Ruff ARG002 warning (update the signature of
Hull.__init__ accordingly and adjust any callers if you remove it).
- Around line 99-107: In the except Exception as e block that currently logs the
failure and calls logger.error(e) before raising a RuntimeError, replace the
second logger.error(e) with logger.exception(...) (or combine into a single
logger.exception call) so the traceback is included in the logs; keep the
existing descriptive message about hull construction failure and re-raise the
RuntimeError('Hull construction from vectors failed. See log file') from e.
In @temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py:
- Around line 127-133: The code uses cast('Any', ...) which defeats type
checking; replace these with casts to the actual target types or remove casts
and rely on direct conversion: for example, change cast('Any',
all_options.get('num_workers', 1)) to either cast(int,
all_options.get('num_workers', 1)) or simply int(all_options.get('num_workers',
1)), and do the same for iteration_limit (int), time_limit_hrs (float), and
cost_epsilon (float) when reading from mga_inputs; alternatively, annotate
mga_inputs/all_options with a TypedDict or a typed config class so you can avoid
casts altogether and use strongly typed access for num_workers, iteration_limit,
time_limit_hrs, and cost_epsilon.
In
@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py:
- Around line 147-148: The method group_variable_names currently ignores its
tech parameter and returns all keys from category_mapping; update it to filter
and return only the variable names for the specified technology (use the tech
argument to look up the appropriate entry in self.category_mapping, e.g.,
self.category_mapping[tech] or iterate items and select keys whose value matches
tech), and return their string names; if the intended behavior is to
deliberately ignore tech, rename the parameter to _tech and keep current
behavior while adding a clarifying comment.
In @temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py:
- Line 4: The importlib.resources call using
resources.files('data_files.example_dbs') fails because data_files and its
subdirectory are not importable packages and the code also references a
non-existent file 'utopia.sqlite'; either make data_files and
data_files/example_dbs into proper packages by adding __init__.py files so
resources.files('data_files.example_dbs') can locate assets, or move the example
DB files into the temoa package (and update pyproject.toml) and change the
resources.files argument to the new package name; alternatively, replace
resources.files(...) with a filesystem-relative loader (using
Path(__file__).parent / 'data_files' / 'example_dbs' / 'utopia.sql' or similar)
and correct the filename to 'utopia.sql' where the code currently expects
'utopia.sqlite'.
In @temoa/extensions/monte_carlo/mc_run.py:
- Around line 224-228: In MCRunFactory.__init__, validate
self.config.monte_carlo_inputs before using it: check that
self.config.monte_carlo_inputs is not None and contains the 'run_settings' key
(mirroring the MCSequencer.__init__ pattern) and raise a clear ValueError or
TypeError if missing; only then set self.settings_file =
Path(self.config.monte_carlo_inputs['run_settings']) and remove the # type:
ignore. Ensure the check references monte_carlo_inputs and the run_settings key
and updates the constructor to fail fast with a descriptive error message when
inputs are absent.
In @temoa/extensions/myopic/myopic_sequencer.py:
- Around line 570-576: The DELETE uses f-string interpolation of the table
identifier in myopic_sequencer.py (loop over self.tables_with_period) which
risks SQL injection; instead validate the table variable against a strict
allowlist (e.g., confirm table in self.tables_with_period or a dedicated
constant) before constructing the query and raise/skip if not allowed, then
build the SQL using the validated table name only; keep parameterized
placeholders for period and scenario (execute(sql, (period, scenario_name))) so
only the identifier is validated while values remain parameterized.
In @temoa/extensions/single_vector_mga/output_summary.py:
- Around line 38-50: The delta calculation in the Emission loop uses an
unnecessary float() wrapper for row_delta while the Activity and Capacity loops
compute row_delta without it; remove the float(...) around ((option - orig) /
orig * 100) in the Emission block so row_delta is computed consistently (as in
the Activity and Capacity loops) when all((orig, option)) is truthy; update the
expression where row_delta is set in the Emission loop (the line that currently
reads row_delta = float((option - orig) / orig * 100) if all((orig, option))
else None) to match the others.
- Around line 31-52: The code incorrectly treats zero values as missing by using
all((orig, option)); update the polling functions (poll_emission, poll_activity,
poll_capacity) to return None for missing data (float | None) and change the
checks in summarize to explicit None checks (e.g., if orig is not None and
option is not None) before computing row_delta; ensure you compute row_delta as
((option - orig) / orig * 100) with a float conversion where needed and leave
row_delta as None when either value is None, then append records as before.
- Around line 31-47: The cast('Iterable[Any]', ...) calls around
emission_labels, activity_labels, and capacity_labels are unnecessary and
verbose; remove the casts in the three sorted(...) iterations (the lines that
call sorted(cast('Iterable[Any]', emission_labels)) and similar for
activity_labels and capacity_labels) and simply call sorted(emission_labels),
sorted(activity_labels), and sorted(capacity_labels) instead, or alternatively
tighten the type of svmga_inputs so those variables are already typed as
Sequence/Iterable[str] to avoid needing casts; keep the existing logic using
poll_emission and poll_activity unchanged.
In @temoa/extensions/stochastics/scenario_creator.py:
- Around line 43-50: The loop contains a standalone type annotation "item:
LoadItem" before the for loop which is unconventional; remove that line and
either rely on the type of hybrid_loader.manifest so the loop variable item is
inferred (for item in hybrid_loader.manifest) or, if necessary, annotate the
manifest's type so item is typed implicitly; update the code around
table_index_map and the for loop to drop the separate "item: LoadItem"
declaration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
pyproject.tomltemoa/extensions/get_comm_tech.pytemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/method_of_morris/morris_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/hull.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/example_builds/scenario_analyzer.pytemoa/extensions/monte_carlo/mc_run.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/myopic/myopic_index.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.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`.
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 165
File: temoa/components/time.py:229-236
Timestamp: 2025-10-18T15:54:24.846Z
Learning: In PR #165 (typing first pass), unused parameter renaming (e.g., `p` → `_p`) is intentionally deferred to a future PR. The current PR focuses exclusively on adding type annotations, not on parameter naming conventions or marking unused parameters.
📚 Learning: 2025-11-03T22:55:02.519Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 186
File: temoa/_internal/temoa_sequencer.py:60-63
Timestamp: 2025-11-03T22:55:02.519Z
Learning: In `TemoaSequencer.__init__`, when a `mode_override` is provided, it must be written back to `self.config.scenario_mode` (not just stored in `self.temoa_mode`) because `HybridLoader.create_data_dict()` has strict consistency checks that validate `config.scenario_mode` matches the presence/absence of `myopic_index`. Without this synchronization, MYOPIC mode would fail with a RuntimeError when the loader is instantiated.
Applied to files:
temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py
🧬 Code graph analysis (11)
temoa/extensions/myopic/myopic_progress_mapper.py (1)
temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(9-29)
temoa/extensions/single_vector_mga/output_summary.py (1)
temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/method_of_morris/morris_sequencer.py (2)
temoa/_internal/table_writer.py (1)
TableWriter(74-774)temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/stochastics/scenario_creator.py (2)
temoa/_internal/run_actions.py (1)
build_instance(121-155)temoa/data_io/loader_manifest.py (1)
LoadItem(26-59)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (1)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (4)
group_members(243-244)group_variable_names(147-148)process_results(206-234)finalize_tracker(374-381)
temoa/extensions/myopic/myopic_sequencer.py (4)
temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(9-29)temoa/extensions/myopic/myopic_progress_mapper.py (2)
MyopicProgressMapper(10-83)report(52-83)temoa/core/config.py (1)
TemoaConfig(32-344)temoa/_internal/table_writer.py (1)
TableWriter(74-774)
temoa/extensions/method_of_morris/morris_evaluate.py (2)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/method_of_morris/morris.py (1)
evaluate(24-63)
temoa/extensions/modeling_to_generate_alternatives/worker.py (3)
temoa/extensions/monte_carlo/mc_worker.py (1)
run(69-184)temoa/extensions/modeling_to_generate_alternatives/hull.py (1)
update(79-117)temoa/extensions/monte_carlo/mc_run.py (1)
model(202-209)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (3)
temoa/extensions/modeling_to_generate_alternatives/hull.py (2)
Hull(15-169)update(79-117)temoa/extensions/modeling_to_generate_alternatives/mga_constants.py (1)
MgaWeighting(15-17)temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (8)
VectorManager(16-76)expired(47-52)group_variable_names(55-57)random_input_vector_model(60-62)process_results(70-71)groups(36-38)group_members(41-43)finalize_tracker(74-76)
temoa/extensions/modeling_to_generate_alternatives/manager_factory.py (3)
temoa/extensions/modeling_to_generate_alternatives/mga_constants.py (2)
MgaAxis(7-11)MgaWeighting(15-17)temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (1)
TechActivityVectorManager(46-381)temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (1)
VectorManager(16-76)
temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py (2)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (2)
VectorManager(16-76)process_results(70-71)temoa/extensions/modeling_to_generate_alternatives/mga_constants.py (2)
MgaAxis(7-11)MgaWeighting(15-17)
🪛 Ruff (0.14.10)
temoa/extensions/method_of_morris/morris_sequencer.py
138-138: Dynamically typed expressions (typing.Any) are disallowed in start
(ANN401)
204-204: Dynamically typed expressions (typing.Any) are disallowed in mm_samples
(ANN401)
205-205: Dynamically typed expressions (typing.Any) are disallowed in process_results
(ANN401)
temoa/extensions/stochastics/scenario_creator.py
21-21: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
21-21: Dynamically typed expressions (typing.Any) are disallowed in scenario_creator
(ANN401)
temoa/extensions/method_of_morris/morris.py
24-24: Dynamically typed expressions (typing.Any) are disallowed in param_values
(ANN401)
25-25: Unused function argument: k
(ARG001)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py
70-70: Dynamically typed expressions (typing.Any) are disallowed in process_results
(ANN401)
temoa/extensions/myopic/myopic_sequencer.py
97-97: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
560-560: Avoid specifying long messages outside the exception class
(TRY003)
574-574: Possible SQL injection vector through string-based query construction
(S608)
temoa/extensions/method_of_morris/morris_evaluate.py
23-23: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
38-38: Dynamically typed expressions (typing.Any) are disallowed in mm_sample
(ANN401)
39-39: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
61-61: Avoid specifying long messages outside the exception class
(TRY003)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
100-103: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/single_vector_mga/sv_mga_sequencer.py
208-208: Boolean-typed positional argument in function definition
(FBT001)
208-208: Boolean default positional argument in function definition
(FBT002)
208-208: Unused static method argument: verbose
(ARG004)
temoa/extensions/monte_carlo/mc_run.py
43-43: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
70-70: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
175-175: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
224-224: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
temoa/extensions/get_comm_tech.py
16-16: Create your own exception
(TRY002)
16-16: Avoid specifying long messages outside the exception class
(TRY003)
19-19: Create your own exception
(TRY002)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
35-35: Possible SQL injection vector through string-based query construction
(S608)
51-51: Create your own exception
(TRY002)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Create your own exception
(TRY002)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Boolean-typed positional argument in function definition
(FBT001)
142-142: Boolean-typed positional argument in function definition
(FBT001)
209-209: Do not catch blind exception: Exception
(BLE001)
263-263: Dynamically typed expressions (typing.Any) are disallowed in get_info
(ANN401)
temoa/extensions/modeling_to_generate_alternatives/worker.py
35-35: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
117-117: datetime.datetime.now() called without a tz argument
(DTZ005)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py
147-147: Unused method argument: tech
(ARG002)
temoa/extensions/modeling_to_generate_alternatives/hull.py
28-28: Unused method argument: kwargs
(ARG002)
28-28: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
101-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
106-106: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/modeling_to_generate_alternatives/manager_factory.py
24-24: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/monte_carlo/mc_sequencer.py
55-55: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
temoa/extensions/monte_carlo/mc_worker.py
46-46: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
54-54: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
159-159: datetime.datetime.now() called without a tz argument
(DTZ005)
⏰ 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 (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
🔇 Additional comments (79)
temoa/extensions/myopic/myopic_index.py (1)
19-19: LGTM!The
-> Nonereturn type annotation is correct for__post_init__. This is a straightforward and accurate type annotation addition consistent with the PR's typing objectives.temoa/extensions/single_vector_mga/sv_mga_sequencer.py (4)
12-12: LGTM! Proper use of TYPE_CHECKING for import.Moving
DataPortalinto theTYPE_CHECKINGblock avoids a runtime import dependency while preserving type information for static analysis. This is the recommended pattern.Also applies to: 26-28
52-53: LGTM! Defensive configuration access with explicit type conversion.The fallback to an empty dict and explicit
float()cast ensures robustness againstNonevalues and incorrect types in the config.
58-58: LGTM!Adding
-> Nonereturn type annotation is good practice for methods that don't return a value.
182-184: LGTM! Clear type annotations for the return structure.The signature accurately describes the function's contract: accepting a model and a tuple, returning two lists of tuples.
temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py (1)
17-19: LGTM - Good extraction to a named variable.Extracting the values into
obj_values_tupleimproves readability and avoids repeating the generator expression. The histogram invocation is correct.Minor consideration: if the query returns no results,
len(obj_values_tuple)will be 0, resulting inbins=0. While this is unlikely in practice for example code,plt.histmay raise a warning or behave unexpectedly with zero bins.temoa/extensions/stochastics/scenario_creator.py (4)
1-18: LGTM!The imports are well-organized with appropriate use of
TYPE_CHECKINGguards for type-only imports, and the# type: ignore[import-untyped]comment is justified since mpi-sppy lacks type stubs.
75-92: LGTM!Good use of
strict=Trueinzip()to catch potential length mismatches betweenindex_colsandidx_tuple. The perturbation logic is clear and handles all action types appropriately.
94-133: LGTM!The instance building logic is well-structured with appropriate validation. The check for empty
first_stage_vars(lines 117-124) prevents cryptic downstream errors in mpi-sppy, and the probability fallback with warning is a sensible default.
57-62: Appropriate use ofcast()for type narrowing.The cast appropriately narrows the type from the untyped
data_dict.get()return value. The string literal'dict[Any, Any] | None'is valid withfrom __future__ import annotations.pyproject.toml (1)
144-144: LGTM!The mypy exclusion pattern is correctly narrowed to only exclude the breakeven extension, enabling type checking for all other typed extensions in this PR.
temoa/extensions/stochastics/stochastic_sequencer.py (2)
1-12: LGTM!The typing scaffolding is correctly implemented:
from __future__ import annotationsenables forward referencesTYPE_CHECKINGguard properly isolatesTemoaConfigimport to avoid runtime import cycles while still supporting type hints
44-44: LGTM!The
type: ignore[import-untyped]comment is appropriate for thempi-sppylibrary which lacks type stubs.temoa/extensions/stochastics/stochastic_config.py (2)
1-9: LGTM!The typing setup is correct:
from __future__ import annotationsenables postponed evaluation, soPathin the type hint (line 38) is treated as a string at runtimePathunderTYPE_CHECKINGis only needed for static analysisSelfis appropriate for the classmethod return type
38-38: Good use ofSelffor the classmethod return type.Using
Selfinstead of the hardcoded'StochasticConfig'string allows proper typing if this class is subclassed, which is the idiomatic pattern for factory classmethods in Python 3.11+.temoa/extensions/myopic/myopic_progress_mapper.py (1)
11-19: LGTM!The type annotations are correct and use modern Python 3.9+ generic syntax (
list[int]), which is appropriate given the project's Python 3.12 minimum requirement.temoa/extensions/single_vector_mga/output_summary.py (2)
1-16: LGTM!The typing scaffolding follows the project's established patterns with
TYPE_CHECKINGguards and appropriatetype: ignorefor the untypedtabulatelibrary.
64-94: LGTM!The
poll_*functions have propersqlite3.Connectiontyping and safeNonehandling for database results.temoa/extensions/myopic/myopic_sequencer.py (6)
66-76: LGTM on class attribute declarations.The explicit class attribute type hints improve code clarity and enable better static analysis.
78-113: LGTM on__init__typing enhancements.The explicit
-> Nonereturn type and properly annotated instance variables align with Python typing best practices.
120-121: Guard added for uninitialized config.Good defensive check before accessing
configattributes.
185-200: Appropriate runtime validation for None states.These guards prevent potential
AttributeErrororTypeErrorwhenconfig,optimization_periods, oridxare uninitialized during the control loop.
418-433: Consistentscenario_namederivation pattern.Using
self.config.scenario if self.config else Noneconsistently handles the case where config may be None.
559-560: Good validation foroptimization_periods.This guard ensures the method fails fast with a clear error message if called before initialization.
temoa/extensions/get_comm_tech.py (5)
1-9: LGTM on imports and future annotations.The
from __future__ import annotationsenables PEP 563 postponed evaluation, which works well with Python 3.12+.
47-71: Type annotations correctly applied toget_scenario.The function signature and internal type hints are appropriate.
74-76: Type hints added forget_commandget_tech.The internal variable annotations (
dict[str, str],set[str]) improve clarity.Also applies to: 142-144
203-210: Return type and exception handling foris_db_overwritten.The
-> boolreturn type is correct. The bareexcept Exceptionat line 209 is broad but acceptable here since it's catching database connection failures.
263-264: Return typeAnyis appropriate forget_info.Given that this function returns different types (
OrderedDict,dict) based on flags,Anyis a reasonable choice. AUnionorTypeVarcould be more precise but would add complexity.temoa/extensions/modeling_to_generate_alternatives/manager_factory.py (3)
1-14: LGTM on TYPE_CHECKING scaffolding.Imports under
TYPE_CHECKINGcorrectly avoid runtime circular dependencies while preserving type information for static analysis.
19-36: Well-structured factory function with proper type safety.The explicit return type
VectorManager, runtime validation forNoneconnection, and**kwargs: Anypattern are appropriate for this factory function.
37-38: Default case ensures exhaustive axis handling.The
case _withNotImplementedErrorprovides clear feedback when unsupported axes are requested.temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (4)
1-14: LGTM on ABC typing setup.The
TYPE_CHECKINGblock correctly guards imports that are only needed for type annotations, avoiding runtime overhead while enabling static type checking.
16-32: Abstract__init__with explicit return type.The
-> Nonereturn type on the abstract initializer aligns with typing best practices.
40-57: Abstract method return types properly annotated.The
list[str]return types forgroup_membersandgroup_variable_namesmatch the concrete implementations inTechActivityVectorManager.
69-76: Return typeAnyforprocess_resultsis pragmatic.While
list[float]would be more precise based onTechActivityVectorManager,Anyallows flexibility for future implementations that may return different types. Thefinalize_trackerwithpassbody after@abstractmethodis valid Python for ABCs.temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py (5)
4-26: LGTM on imports and TYPE_CHECKING setup.The
TYPE_CHECKINGblock properly guards imports needed only for type annotations, includingBaseProcess,Results,DataPortal, and internal types.
54-69: Class attribute declarations improve type safety.Explicit class-level type hints enable better IDE support and static analysis for this complex sequencer class.
222-228: Queue type annotations withAnyare appropriate.Given the queues transport heterogeneous data (models, shutdown signals, log records),
Queue[Any]is a reasonable choice.
270-270: Keyword argumentmodel=aligns withVectorManager.process_resultssignature.The explicit keyword argument improves readability and matches the abstract method's parameter name.
Also applies to: 314-314
152-152: Explicit-> Nonereturn types on lifecycle methods.The
start,process_solve_results, and__del__methods now have explicit return types, improving consistency across the codebase.Also applies to: 365-365, 380-380
temoa/extensions/modeling_to_generate_alternatives/worker.py (6)
4-11: LGTM! Future annotations and typing imports.The
from __future__ import annotationsimport enables postponed evaluation of annotations (PEP 563), which is appropriate for forward references and cleaner type hints.
22-33: LGTM! Class-level attribute annotations.The explicit attribute declarations improve type checking and documentation. The types align with the corresponding
__init__parameters and usage throughout the class.
61-68: LGTM! Typed run method and logger.The explicit
-> Nonereturn type andlogger: logging.Loggerannotation improve type safety. This pattern aligns with themc_worker.pyimplementation.
84-87: LGTM! Variable rename for clarity.Renaming to
log_location_strclearly indicates the string conversion purpose before passing to solver options. This improves readability.
103-116: LGTM! Typed solve result with consistent naming.The
solve_res: SolverResults | Noneannotation and assignment toNoneon exception aligns with the pattern inmc_worker.py, providing consistent error handling across workers.
120-137: LGTM! Updated references to usesolve_res.The termination check and status extraction correctly use the renamed
solve_resvariable, maintaining consistency with the exception handling above.temoa/extensions/modeling_to_generate_alternatives/hull.py (5)
4-10: LGTM! Future annotations and typing imports.The
# type: ignore[import-untyped]comment for scipy.spatial is appropriate since SciPy doesn't provide complete type stubs.
16-26: LGTM! Class-level attribute annotations.The explicit attribute declarations with proper types (
np.ndarray | None,Anyfor cv_hull) provide clear documentation of the class state.
75-77: LGTM! Division-by-zero guard.Good defensive addition to return
0.0whennorms_checked == 0, preventing potentialZeroDivisionError.
137-144: LGTM! Defensive null check inget_norm.The added guard
if self._valid_norms is not Noneprevents potentialTypeErrorwhen accessing an uninitialized array.
148-152: LGTM! Defensive null check inget_all_norms.The guard ensures safe slicing and returns an empty array when no norms are available.
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (8)
1-24: LGTM! Well-organized TYPE_CHECKING imports.The deferred imports under
TYPE_CHECKINGavoid circular imports and runtime overhead while enabling full type checking. Thecollections.abctypes (Iterable,Iterator,Mapping,Sequence) are appropriate for type hints.
34-38: LGTM! Return type annotations for__str__and__repr__.Adding explicit
-> strreturn types completes the type annotations forDefaultItem.
46-62: LGTM! Comprehensive class-level attribute annotations.The explicit type declarations provide clear documentation of the class state and enable better static analysis.
64-109: LGTM! Typed__init__with proper defaults.The
-> Nonereturn type and explicit initialization ofhull_points = Noneandhull = Nonealign with the class attribute declarations.
133-140: LGTM! Safe iteration withor set()pattern.Using
or set()provides a safe fallback whenactive_flow_rpsditvooractive_flow_rpitvomight beNone, preventing iteration errors.
307-322: LGTM! Defensive None guard inregenerate_hull.The early return with a warning when
hull_points is Noneprevents attempting hull construction with no data.
331-360: LGTM! Typed static method withMapping/Sequencegenerics.Using
Mapping[Any, Sequence[str]]andMapping[str, int]for parameters allows flexibility while maintaining type safety.
362-372: LGTM! Defensive None guards intracker.The combined check
if self.hull is not None and self.hull_points is not Noneensures the tracker only operates when valid data exists.temoa/extensions/method_of_morris/morris_evaluate.py (3)
5-20: LGTM on typing imports and TYPE_CHECKING pattern.The use of
TYPE_CHECKINGto guard theTemoaConfigimport avoids circular imports at runtime while still enabling static type checking. This is the correct pattern for forward references.
23-35: LGTM on type annotations.The
Anytype forlog_queueis appropriate here since multiprocessing queue types vary and aren't well-typed in the standard library.
91-107: LGTM on variable renaming and type consistency.The snake_case naming convention aligns with Python standards, and the explicit
float()casts ensure the return typelist[float]is honored even when database values might be integers.temoa/extensions/method_of_morris/morris_sequencer.py (5)
17-21: LGTM ontype: ignorecomments for untyped libraries.The
# type: ignore[import-untyped]annotations are appropriate for external libraries (joblib, SALib) that lack type stubs. This silences type checker warnings while maintaining type safety in the rest of the codebase.
138-202: LGTM onstartmethod typing.The
-> Anyreturn type is pragmatic given that the return value comes from SALib'smorris.analyze(), which is untyped. Documenting the actual return structure in a docstring would be helpful for consumers but isn't required.
204-205: LGTM onprocess_resultstype hints.The
mm_samples: Anyandmorris_results: list[Any]types are appropriate given these come from SALib's sample function and the evaluate function respectively.
276-349: LGTM ongather_parametersreturn type.The return type
dict[int, list[Any]]accurately reflects the data structure being built and is consistent with theparam_infoparameter type inmorris_evaluate.py.
351-352: LGTM on__del__return type.Adding
-> Noneto__del__follows the project's typing conventions as seen inTableWriter.__del__in the relevant code snippets.temoa/extensions/method_of_morris/morris.py (3)
1-7: LGTM on import additions.The new imports support the type annotations and are properly organized with
from __future__ import annotationsat the top.
54-63: LGTM on variable naming updates.The snake_case naming (
y_of,y_cumulative_co2,morris_objectives) is consistent with Python conventions and aligns with the naming inmorris_evaluate.py.
160-203: LGTM on analysis result variable renaming.Renaming to
si_ofandsi_cumulative_co2provides clearer semantics (sensitivity indices) and follows snake_case conventions.temoa/extensions/monte_carlo/mc_worker.py (2)
131-163: LGTM on solve result handling.The
solve_resvariable is properly typed asSolverResults | Noneand all code paths (success branches and exception handler) assign to it before use. Thetry/except AttributeErrorblock at lines 162-183 correctly guards againstNonebeing passed tocheck_optimal_termination.
34-44: Good use of class-level attribute annotations.The explicit attribute declarations improve IDE support and static analysis. This pattern aligns well with the typing effort across the Monte Carlo modules.
temoa/extensions/monte_carlo/mc_sequencer.py (3)
269-277: LGTM on cast usage in shutdown handling.The guard at line 275 (
next_result is not None and next_result != 'COYOTE') correctly ensures thatnext_resultis aDataBrickbefore the cast on line 277. This aligns with the queue typeQueue[DataBrick | str]where the only string value is the'COYOTE'shutdown signal.
24-32: Good use ofTYPE_CHECKINGfor deferred imports.Moving
DataPortal,DataBrick,TemoaConfig, and multiprocessing types underTYPE_CHECKINGcorrectly avoids runtime import overhead and potential circular dependencies while maintaining full type annotation support.
143-152: Well-typed queue and worker container declarations.The explicit type annotations for
work_queue,result_queue,log_queue, andworkersimprove code clarity and enable better static analysis across the multiprocessing workflow.temoa/extensions/monte_carlo/mc_run.py (3)
253-264: LGTM on Generator type annotations.The
Generator[tuple[int, str]]return type correctly uses thecollections.abc.Generatorshorthand form, which is valid when only specifying the yield type (with implicitNonefor send and return types).
15-21: Good use ofTYPE_CHECKINGfor type-only imports.The
Generator,DataPortal, andTemoaConfigimports underTYPE_CHECKINGcorrectly avoid runtime import overhead while enabling full type annotation support. This is consistent with the pattern used inmc_worker.pyandmc_sequencer.py.
362-368: Improved data access pattern with intermediate variable.Extracting
param_vals = data_store[tweak.param_name]into a local variable improves readability and reduces repeated dictionary lookups when accessing and modifying the same parameter data.
| setting_entry = 'run # %d: Setting param %s[%s] to value: %f' | ||
| log_entry.append(setting_entry) | ||
| logger.debug('\n '.join(log_entry)) |
There was a problem hiding this comment.
Log format string is never interpolated with actual values.
The setting_entry format string contains placeholders (%d, %s, %s, %f) but is appended to log_entry without being formatted. The debug log will output the raw format string instead of the actual parameter values.
🐛 Proposed fix
- setting_entry = 'run # %d: Setting param %s[%s] to value: %f'
- log_entry.append(setting_entry)
+ setting_entry = f'run # {i + 1}: Setting param {param_name}[{set_idx_tuple}] to value: {mm_sample[j]}'
+ log_entry.append(setting_entry)🤖 Prompt for AI Agents
In @temoa/extensions/method_of_morris/morris_evaluate.py around lines 65 - 67,
The format string stored in setting_entry is never interpolated before being
appended to log_entry, so the log prints the raw "%d/%s/%f" placeholders; update
the code where setting_entry is appended to instead append a formatted string
(e.g., use setting_entry % (run_number, param_name, param_index, param_value) or
construct an f-string) and ensure the variables used match the expected types,
then call logger.debug on the joined log_entry as before.
| def evaluate(param_names: dict[int, list[Any]], param_values: Any, | ||
| data: dict[str, Any], k: int) -> list[Any]: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused parameter k should be prefixed with underscore.
The k parameter is not used in the function body. Per Python convention, prefix it with _ to indicate it's intentionally unused.
♻️ Proposed fix
def evaluate(param_names: dict[int, list[Any]], param_values: Any,
- data: dict[str, Any], k: int) -> list[Any]:
+ data: dict[str, Any], _k: int) -> list[Any]:📝 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.
| def evaluate(param_names: dict[int, list[Any]], param_values: Any, | |
| data: dict[str, Any], k: int) -> list[Any]: | |
| def evaluate(param_names: dict[int, list[Any]], param_values: Any, | |
| data: dict[str, Any], _k: int) -> list[Any]: |
🧰 Tools
🪛 Ruff (0.14.10)
24-24: Dynamically typed expressions (typing.Any) are disallowed in param_values
(ANN401)
25-25: Unused function argument: k
(ARG001)
🤖 Prompt for AI Agents
In @temoa/extensions/method_of_morris/morris.py around lines 24 - 25, The
evaluate function has an unused parameter k; rename it to _k in the function
signature (def evaluate(param_names: dict[int, list[Any]], param_values: Any,
data: dict[str, Any], _k: int) -> list[Any]) to follow Python convention for
intentionally unused parameters and update any internal references to use _k if
present.
| except Exception as e: | ||
| # scipy.spatial._qhull.QhullError may not be easily found by mypy | ||
| logger.error( | ||
| 'Attempt at hull construction from basis vectors failed.' | ||
| '\nMay be non-recoverable. Possibly try a set of random vectors to initialize the ' | ||
| 'Hull.' | ||
| ) | ||
| logger.error(e) | ||
| raise RuntimeError('Hull construction from vectors failed. See log file') | ||
| raise RuntimeError('Hull construction from vectors failed. See log file') from e |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use logger.exception instead of logger.error for exception logging.
logger.exception() automatically includes the traceback, providing more diagnostic information. Per Ruff TRY400, this is the recommended pattern when logging within an exception handler.
Proposed fix
except Exception as e:
# scipy.spatial._qhull.QhullError may not be easily found by mypy
- logger.error(
+ logger.exception(
'Attempt at hull construction from basis vectors failed.'
'\nMay be non-recoverable. Possibly try a set of random vectors to initialize the '
'Hull.'
)
- logger.error(e)
raise RuntimeError('Hull construction from vectors failed. See log file') from e🧰 Tools
🪛 Ruff (0.14.10)
101-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
106-106: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @temoa/extensions/modeling_to_generate_alternatives/hull.py around lines 99 -
107, In the except Exception as e block that currently logs the failure and
calls logger.error(e) before raising a RuntimeError, replace the second
logger.error(e) with logger.exception(...) (or combine into a single
logger.exception call) so the traceback is included in the logs; keep the
existing descriptive message about hull construction failure and re-raise the
RuntimeError('Hull construction from vectors failed. See log file') from e.
| for item in sorted(cast('Iterable[Any]', emission_labels)): | ||
| orig = poll_emission( | ||
| conn, | ||
| scenarios[0], | ||
| item, | ||
| ) | ||
| option = poll_emission(conn, scenarios[1], item) | ||
| delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, delta]) | ||
| for item in sorted(activity_labels): | ||
| row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, row_delta]) | ||
| for item in sorted(cast('Iterable[Any]', activity_labels)): | ||
| orig = poll_activity(conn, scenarios[0], item) | ||
| option = poll_activity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
|
|
||
| records.append(['Activity', item, orig, option, delta]) | ||
| records.append(['Activity', item, orig, option, row_delta]) | ||
|
|
||
| for item in sorted(capacity_labels): | ||
| for item in sorted(cast('Iterable[Any]', capacity_labels)): | ||
| orig = poll_capacity(conn, scenarios[0], item) | ||
| option = poll_capacity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
|
|
||
| records.append(['Capacity', item, orig, option, delta]) | ||
| records.append(['Capacity', item, orig, option, row_delta]) |
There was a problem hiding this comment.
Zero values are incorrectly treated as missing data.
The all((orig, option)) check treats 0.0 as falsy, so legitimate zero emissions/activity/capacity values will result in row_delta = None instead of computing the percentage change. Since the poll_* functions return 0.0 for missing data, there's no way to distinguish between "no data" and "actual zero".
🐛 Suggested fix using explicit None checks
Modify the poll_* functions to return float | None and check for None explicitly:
-def poll_emission(conn: sqlite3.Connection, scenario: str, label: str) -> float:
+def poll_emission(conn: sqlite3.Connection, scenario: str, label: str) -> float | None:
...
row = conn.execute(...).fetchone()
- return float(row[0] if row and row[0] is not None else 0.0)
+ return float(row[0]) if row and row[0] is not None else NoneThen in summarize:
- row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None
+ row_delta = (option - orig) / orig * 100 if orig is not None and option is not None and orig != 0 else None🤖 Prompt for AI Agents
In @temoa/extensions/single_vector_mga/output_summary.py around lines 31 - 52,
The code incorrectly treats zero values as missing by using all((orig, option));
update the polling functions (poll_emission, poll_activity, poll_capacity) to
return None for missing data (float | None) and change the checks in summarize
to explicit None checks (e.g., if orig is not None and option is not None)
before computing row_delta; ensure you compute row_delta as ((option - orig) /
orig * 100) with a float conversion where needed and leave row_delta as None
when either value is None, then append records as before.
| for item in sorted(cast('Iterable[Any]', emission_labels)): | ||
| orig = poll_emission( | ||
| conn, | ||
| scenarios[0], | ||
| item, | ||
| ) | ||
| option = poll_emission(conn, scenarios[1], item) | ||
| delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, delta]) | ||
| for item in sorted(activity_labels): | ||
| row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, row_delta]) | ||
| for item in sorted(cast('Iterable[Any]', activity_labels)): | ||
| orig = poll_activity(conn, scenarios[0], item) | ||
| option = poll_activity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
|
|
||
| records.append(['Activity', item, orig, option, delta]) | ||
| records.append(['Activity', item, orig, option, row_delta]) | ||
|
|
||
| for item in sorted(capacity_labels): | ||
| for item in sorted(cast('Iterable[Any]', capacity_labels)): |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The cast('Iterable[Any]', ...) calls are verbose and could be simplified.
Since svmga_inputs.get('emission_labels', []) returns a list (which is already Iterable), and you're iterating over it with sorted(), the cast adds verbosity without benefit. Consider typing svmga_inputs more precisely or removing the casts.
♻️ Alternative: type the labels directly
- emission_labels = svmga_inputs.get('emission_labels', [])
- capacity_labels = svmga_inputs.get('capacity_labels', [])
- activity_labels = svmga_inputs.get('activity_labels', [])
+ emission_labels: list[str] = svmga_inputs.get('emission_labels', [])
+ capacity_labels: list[str] = svmga_inputs.get('capacity_labels', [])
+ activity_labels: list[str] = svmga_inputs.get('activity_labels', [])
...
- for item in sorted(cast('Iterable[Any]', emission_labels)):
+ for item in sorted(emission_labels):🤖 Prompt for AI Agents
In @temoa/extensions/single_vector_mga/output_summary.py around lines 31 - 47,
The cast('Iterable[Any]', ...) calls around emission_labels, activity_labels,
and capacity_labels are unnecessary and verbose; remove the casts in the three
sorted(...) iterations (the lines that call sorted(cast('Iterable[Any]',
emission_labels)) and similar for activity_labels and capacity_labels) and
simply call sorted(emission_labels), sorted(activity_labels), and
sorted(capacity_labels) instead, or alternatively tighten the type of
svmga_inputs so those variables are already typed as Sequence/Iterable[str] to
avoid needing casts; keep the existing logic using poll_emission and
poll_activity unchanged.
| row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None | ||
| records.append(['Emission', item, orig, option, row_delta]) | ||
| for item in sorted(cast('Iterable[Any]', activity_labels)): | ||
| orig = poll_activity(conn, scenarios[0], item) | ||
| option = poll_activity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
|
|
||
| records.append(['Activity', item, orig, option, delta]) | ||
| records.append(['Activity', item, orig, option, row_delta]) | ||
|
|
||
| for item in sorted(capacity_labels): | ||
| for item in sorted(cast('Iterable[Any]', capacity_labels)): | ||
| orig = poll_capacity(conn, scenarios[0], item) | ||
| option = poll_capacity(conn, scenarios[1], item) | ||
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | ||
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent float() wrapper in delta calculations.
Line 38 wraps the result in float() while lines 43 and 50 do not. Since division in Python 3 always returns float, the wrapper is unnecessary but the inconsistency is confusing.
♻️ Remove redundant float() for consistency
- row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None
+ row_delta = (option - orig) / orig * 100 if all((orig, option)) else None📝 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.
| row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None | |
| records.append(['Emission', item, orig, option, row_delta]) | |
| for item in sorted(cast('Iterable[Any]', activity_labels)): | |
| orig = poll_activity(conn, scenarios[0], item) | |
| option = poll_activity(conn, scenarios[1], item) | |
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| records.append(['Activity', item, orig, option, delta]) | |
| records.append(['Activity', item, orig, option, row_delta]) | |
| for item in sorted(capacity_labels): | |
| for item in sorted(cast('Iterable[Any]', capacity_labels)): | |
| orig = poll_capacity(conn, scenarios[0], item) | |
| option = poll_capacity(conn, scenarios[1], item) | |
| delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| records.append(['Emission', item, orig, option, row_delta]) | |
| for item in sorted(cast('Iterable[Any]', activity_labels)): | |
| orig = poll_activity(conn, scenarios[0], item) | |
| option = poll_activity(conn, scenarios[1], item) | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None | |
| records.append(['Activity', item, orig, option, row_delta]) | |
| for item in sorted(cast('Iterable[Any]', capacity_labels)): | |
| orig = poll_capacity(conn, scenarios[0], item) | |
| option = poll_capacity(conn, scenarios[1], item) | |
| row_delta = (option - orig) / orig * 100 if all((orig, option)) else None |
🤖 Prompt for AI Agents
In @temoa/extensions/single_vector_mga/output_summary.py around lines 38 - 50,
The delta calculation in the Emission loop uses an unnecessary float() wrapper
for row_delta while the Activity and Capacity loops compute row_delta without
it; remove the float(...) around ((option - orig) / orig * 100) in the Emission
block so row_delta is computed consistently (as in the Activity and Capacity
loops) when all((orig, option)) is truthy; update the expression where row_delta
is set in the Emission loop (the line that currently reads row_delta =
float((option - orig) / orig * 100) if all((orig, option)) else None) to match
the others.
ParticularlyPythonicBS
force-pushed
the
typing/extensions
branch
from
74c5f32 to
ab26865
Compare
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (32)
temoa/extensions/myopic/myopic_progress_mapper.py (1)
11-19: Guard against emptysorted_future_years(currently crashes inmax(...)).
Even if “shouldn’t happen”, failing with a targeted error makes debugging/config issues much easier.Proposed fix
class MyopicProgressMapper: def __init__(self, sorted_future_years: list[int]) -> None: + if not sorted_future_years: + raise ValueError('sorted_future_years must be non-empty') self.leader = '--' self.trailer = ''.join(reversed(self.leader)) self.years = sorted_future_years self.tag_width = max(len(str(t)) for t in sorted_future_years) + 2 * len(self.leader)temoa/extensions/myopic/myopic_sequencer.py (1)
154-160: Bug:start()’sself.config is Noneguard is too late to prevent crashes.
start()callsself.characterize_run()before the guard; withconfig=Nonethis can fail via missingcursor/view_depth/etc.long before your intendedRuntimeError.Proposed fix (move guard to the top of `start()`)
def start(self) -> None: + if self.config is None: + raise RuntimeError('Config is not initialized') # noqa: TRY003 + # load up the instance queue self.characterize_run() @@ - if self.config is None: - raise RuntimeError('Config is not initialized') - logger.info('Starting Myopic Sequence')Also applies to: 185-187
temoa/extensions/monte_carlo/mc_sequencer.py (2)
110-231: Fix unhandledStopIterationwhen primingrun_gen(can crashstart()and leak workers).
mc_run: MCRun = next(run_gen)(Line 179) will raise ifrun_generator()yields nothing (e.g., all runs aborted due to “no good tweaks”), but workers have already been spawned (Lines 161-174). This can terminate the sequencer without sending shutdown signals / joining processes.Proposed fix (guard initial next() and avoid starting workers when there are no runs)
@@ - # 3. set up the run generator - run_gen = mc_factory.run_generator() + # 3. set up the run generator + run_gen = mc_factory.run_generator() + + # Prime generator before starting workers so we can exit cleanly if there are no runs. + try: + mc_run: MCRun = next(run_gen) + except StopIteration: + logger.warning("Monte Carlo run generator produced no runs; nothing to do.") + return @@ - # 4. Set up the workers + # 4. Set up the workers import multiprocessing ctx = multiprocessing.get_context('spawn') @@ - # pull the first instance - mc_run: MCRun = next(run_gen) + # first instance already pulled above
142-261: Make shutdown signaling non-blocking/timeout-bounded to avoid hangs whenwork_queueis full.
work_queue.put('ZEBRA')(Line 259) can block indefinitely because the queue has a small maxsize and may still contain pending work items at shutdown time. If any worker is stalled/crashed, this can deadlock the manager.Proposed fix (timeout + retry)
@@ - for _ in workers: + for _ in workers: if self.verbose: print('shutdown sent') - work_queue.put('ZEBRA') # shutdown signal + # Avoid potential deadlock if the work queue is full. + while True: + try: + work_queue.put('ZEBRA', timeout=1.0) + break + except queue.Full: + time.sleep(0.1) logger.debug('Put "ZEBRA" on work queue (shutdown signal)')temoa/extensions/monte_carlo/mc_worker.py (3)
106-184: Harden solve-result handling: don’t callcheck_optimal_termination()onNone, and avoidsolve_res['Solver']access.
Right now, a failed solve setssolve_res = None(Line 158) but still flows intocheck_optimal_termination(solve_res)(Line 163) and later triessolve_res['Solver']...(Lines 175-181). This can throw (and is currently swallowed by theexcept AttributeError: pass), causing silent loss of failure diagnostics.Proposed fix (explicit None-guard + use standard `SolverResults` attributes)
@@ - # guard against a bad "res" object... - try: - good_solve = check_optimal_termination(solve_res) - if good_solve: + # Guard against missing/invalid solve results. + if solve_res is None: + continue + + try: + good_solve = check_optimal_termination(solve_res) + if good_solve: data_brick = data_brick_factory(model) self.results_queue.put(data_brick) logger.info( 'Worker %d solved a model in %0.2f minutes', self.worker_number, (toc - tic).total_seconds() / 60, ) if verbose: print(f'Worker {self.worker_number} completed a successful solve') else: - if solve_res is not None: - status = solve_res['Solver'].termination_condition - logger.info( - 'Worker %d did not solve. Results status: %s', - self.worker_number, - status, - ) - except AttributeError: - pass + status = getattr(solve_res.solver, "termination_condition", None) + logger.info( + 'Worker %d did not solve. Results termination condition: %s', + self.worker_number, + status, + ) + except Exception: + logger.exception("Unexpected error while interpreting solver results")
46-67: Add__init__ -> Noneand consider replacing**kwargs: Anywith explicit args.
This is both a Ruff violation (ANN204, ANN401) and improves the public surface for type-safety.Proposed fix (minimal: add return type; better: make args explicit)
@@ def __init__( self, dp_queue: Queue[Any], results_queue: Queue[DataBrick | str], log_root_name: str, log_queue: Queue[logging.LogRecord], log_level: int = logging.INFO, solver_log_path: Path | None = None, - **kwargs: Any, - ): + solver_name: str = "", + solver_options: dict[str, Any] | None = None, + ) -> None: @@ - self.solver_name = kwargs['solver_name'] - self.solver_options = kwargs['solver_options'] + self.solver_name = solver_name + self.solver_options = solver_options or {}
106-115: Guardself.opt.options = ...assignment against appsi solvers.Line 107 applies the legacy
.optionsAPI unconditionally, but appsi solvers (appsi_ipopt, appsi_highs, etc.) do not support this interface and will fail or ignore it. This causes the assignment to be ineffective for appsi backends and risks errors in multiprocessing workers. Wrap line 107 withhasattr(self.opt, 'options')or a try/except block. The subsequent per-key config mapping (lines 110–115) is correct but serves only as a partial workaround; the initial unguarded assignment must be skipped for appsi solvers entirely.Note: Update the comment on line 103—appsi solvers do not accept options via
.optionsat all; use.configfor solver-independent settings and.ipopt_options/.highs_options/.solver_optionsfor solver-specific parameters.temoa/extensions/monte_carlo/mc_run.py (5)
201-209: FixMCRun.model: it passes a tuple intocreate_instance()(likely runtime failure).
model_dpreturns(name, dp)(Lines 195-200), butmodelassignsdp = self.model_dp(Line 203) and callscreate_instance(data=dp)(Line 205). Thatdpis atuple[str, DataPortal], not aDataPortal.Proposed fix
@@ def model(self) -> TemoaModel: - dp = self.model_dp + _, dp = self.model_dp model = TemoaModel() instance = model.create_instance(data=dp)
230-251: Don’t useassertfor input validation inprescreen_input_file()(can be optimized away).
Withpython -O, the header check disappears; invalid files will slip through and fail later in less actionable ways.Proposed fix
@@ with open(self.settings_file) as f: header = f.readline().strip() - assert header == 'run,param,index,mod,value,notes', ( - 'header should be: run,param,index,mod,value,notes' - ) + if header != 'run,param,index,mod,value,notes': + raise ValueError('header should be: run,param,index,mod,value,notes')
293-325: Guardelement_locator()against empty param dicts (current code canIndexError).
Ifparam_datais{}(present but empty),tuple(param_data.keys())[0](Line 313) crashes.Proposed fix
@@ param_data = data_store.get(param) if not param_data: return [] + if not getattr(param_data, "keys", None) or not param_data: + return [] @@ - first_index = tuple(param_data.keys())[0] + first_index = next(iter(param_data.keys()))
266-291: Handle empty run-settings files intweak_set_generator()(unguardednext(rows)).
If the CSV has only a header,next(rows)(Line 274) raisesStopIterationand will currently crash the generator consumer (e.g., the sequencer).Proposed fix
@@ rows = self._next_row_generator() empty = False run_tweaks = [] - row_number, next_row = next(rows) + try: + row_number, next_row = next(rows) + except StopIteration: + return
43-55: Add missing__init__ -> Nonereturn types (Ruff ANN204).
Applies toTweak.__init__(Line 43),TweakFactory.__init__(Line 70),MCRun.__init__(Line 175),MCRunFactory.__init__(Line 224).Proposed fix (example pattern)
@@ - def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float): + def __init__( + self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float + ) -> None: @@ - def __init__(self, data_store: dict[str, Any]): + def __init__(self, data_store: dict[str, Any]) -> None: @@ def __init__( @@ - ): + ) -> None: @@ - def __init__(self, config: TemoaConfig, data_store: dict[str, Any]): + def __init__(self, config: TemoaConfig, data_store: dict[str, Any]) -> None:Also applies to: 70-78, 175-186, 224-229
temoa/extensions/method_of_morris/morris_sequencer.py (2)
86-131: Fix Morris option parsing: falsy values + missing int coercions can break runtime.
if pert:/if levels:/if traj:will treat0/0.0as “not provided”. Alsocoresisn’t coerced toint, so'0'or'4'will misbehave ('0' == 0is False; joblibn_jobs='4'will error).Proposed fix
- pert = morris_inputs.get('perturbation') - if pert: - self.mm_perturbation = float(cast(str | float, pert)) + pert = morris_inputs.get('perturbation') + if pert is not None: + self.mm_perturbation = float(pert) logger.info('Morris perturbation: %0.2f', self.mm_perturbation) else: self.mm_perturbation = 0.10 logger.warning( 'No value received for perturbation, using default: %0.2f', self.mm_perturbation ) - levels = morris_inputs.get('levels') - if levels: - self.num_levels = int(cast('Any', levels)) + levels = morris_inputs.get('levels') + if levels is not None: + self.num_levels = int(levels) logger.info('Morris levels: %d', self.num_levels) else: self.num_levels = ( 8 # the number of levels to divide the param range into (must be even number) ) logger.warning('No value received for levels, using default: %d', self.num_levels) - traj = morris_inputs.get('trajectories') - if traj: - self.trajectories = int(cast('Any', traj)) + traj = morris_inputs.get('trajectories') + if traj is not None: + self.trajectories = int(traj) logger.info('Morris trajectories: %d', self.trajectories) else: self.trajectories = 4 # number of morris trajectories to generate logger.warning( 'No value received for trajectories, using default: %d', self.trajectories ) seed = morris_inputs.get('seed') - self.seed = int(cast('Any', seed)) if seed else None + self.seed = int(seed) if seed is not None else None logger.info('Morris Seed (None indicates system generated): %s', self.seed) - self.num_cores = morris_inputs.get('cores', 0) - if self.num_cores == 0: + self.num_cores = int(morris_inputs.get('cores', 0) or 0) + if self.num_cores == 0: self.num_cores = multiprocessing.cpu_count()
138-203: Ensure QueueListener/Manager are cleaned up on exceptions (avoid leaked threads/processes).If any worker raises,
log_listener.stop()won’t run, and themultiprocessing.Manager()server process may linger.Proposed fix
- m = multiprocessing.Manager() - log_queue = m.Queue() # Queue() - - log_listener = QueueListener(log_queue, *logging.root.handlers) - log_level = logger.getEffectiveLevel() - log_listener.start() + with multiprocessing.Manager() as m: + log_queue = m.Queue() + log_listener = QueueListener(log_queue, *logging.root.handlers) + log_level = logger.getEffectiveLevel() + log_listener.start() + try: + # 5. Run the processing: + if not self.config.silent: + msg = f'Starting {len(mm_samples)} MM runs on {self.num_cores} cores.\n' + sys.stdout.write(msg) + sys.stdout.write('=' * (len(msg) - 1) + '\n') + sys.stdout.flush() + morris_results = Parallel(n_jobs=self.num_cores)( + delayed(evaluate)( + param_names, mm_samples[i, :], data, i, self.config, log_queue, log_level + ) + for i in range(0, len(mm_samples)) + ) + finally: + log_listener.stop() - - # 5. Run the processing: - if not self.config.silent: - msg = f'Starting {len(mm_samples)} MM runs on {self.num_cores} cores.\n' - sys.stdout.write(msg) - sys.stdout.write('=' * (len(msg) - 1) + '\n') - sys.stdout.flush() - morris_results = Parallel(n_jobs=self.num_cores)( - delayed(evaluate)( - param_names, mm_samples[i, :], data, i, self.config, log_queue, log_level - ) - for i in range(0, len(mm_samples)) - ) - log_listener.stop()temoa/extensions/method_of_morris/morris_evaluate.py (2)
23-35: Logger handler detection is too broad (hasHandlers()checks ancestors).
hasHandlers()can return True due to parent/root handlers, which can prevent attaching theQueueHandlerand silently drop worker logs into the queue. Prefer checkingworker_logger.handlers(and setpropagate = False) if the intent is “ensure this logger has a QueueHandler”.
79-107: Handle empty objective rows and NULL SUM emissions to avoid runtime crashes.
- If
output_objectivereturns 0 rows,output_query[0][0]raisesIndexError.SELECT SUM(emission) ...commonly returns 1 row withNonewhen there are no matches;float(None)will raise.Proposed fix
cur.execute( 'SELECT total_system_cost FROM output_objective where scenario = ?', (scenario_name,) ) output_query = cur.fetchall() - if len(output_query) > 1: + if len(output_query) == 0: + raise RuntimeError( + 'No output found in output_objective table matching scenario name. Model write failed.' + ) + if len(output_query) > 1: raise RuntimeError( 'Multiple outputs found in Objective table matching scenario name. Coding error.' ) else: y_of = output_query[0][0] ... output_query = cur.fetchall() - if len(output_query) == 0: - y_cumulative_co2 = 0.0 + if len(output_query) == 0: + y_cumulative_co2 = 0.0 elif len(output_query) > 1: raise RuntimeError( 'Multiple outputs found in output_emissions table matching scenario name. Coding ' 'error.' ) else: - y_cumulative_co2 = output_query[0][0] + y_cumulative_co2 = output_query[0][0] or 0.0 morris_objectives = [float(y_of), float(y_cumulative_co2)]temoa/extensions/stochastics/scenario_creator.py (2)
53-56: Avoid reusingpfor both “perturbation” and “period” loop variables.This currently works, but it’s easy to misread and can lead to accidental bugs when this function is edited.
Proposed diff
- for p in stoch_config.perturbations: - if p.scenario != scenario_name: + for perturbation in stoch_config.perturbations: + if perturbation.scenario != scenario_name: continue @@ - target_param = cast(dict[Any, Any] | None, data_dict.get(p.table)) + target_param = cast(dict[Any, Any] | None, data_dict.get(perturbation.table)) @@ - for r, t, p in instance.v_new_capacity: - if p == first_period: - first_stage_vars.append(instance.v_new_capacity[r, t, p]) + for r, t, period in instance.v_new_capacity: + if period == first_period: + first_stage_vars.append(instance.v_new_capacity[r, t, period])Also applies to: 113-116
21-34: UseTypedDict+Unpackto replaceAnyfor**kwargsand specifyTemoaModelas return type.
scenario_creatorhas a stable kwargs contract; encoding it withTypedDict+Unpackimproves type safety, fixes Ruff ANN401, and eliminates the need for runtime validation. The function already returns aTemoaModelfrombuild_instance().Proposed diff
from __future__ import annotations import logging import sqlite3 -from typing import TYPE_CHECKING, Any, cast +from typing import TYPE_CHECKING, TypedDict, cast +from typing import Unpack from mpisppy.utils.sputils import attach_root_node # type: ignore[import-untyped] from temoa._internal.run_actions import build_instance +from temoa.core.model import TemoaModel from temoa.components.costs import period_cost_rule from temoa.data_io.hybrid_loader import HybridLoader if TYPE_CHECKING: from temoa.core.config import TemoaConfig from temoa.data_io.hybrid_loader import LoadItem from temoa.extensions.stochastics.stochastic_config import StochasticConfig logger = logging.getLogger(__name__) +class _ScenarioCreatorKwargs(TypedDict): + temoa_config: TemoaConfig + stoch_config: StochasticConfig + -def scenario_creator(scenario_name: str, **kwargs: Any) -> Any: +def scenario_creator(scenario_name: str, **kwargs: Unpack[_ScenarioCreatorKwargs]) -> TemoaModel: """ Creator for mpi-sppy scenarios. @@ - if 'temoa_config' not in kwargs or 'stoch_config' not in kwargs: - raise ValueError("scenario_creator requires 'temoa_config' and 'stoch_config' in kwargs") - - temoa_config: TemoaConfig = kwargs['temoa_config'] - stoch_config: StochasticConfig = kwargs['stoch_config'] + temoa_config = kwargs["temoa_config"] + stoch_config = kwargs["stoch_config"]mpi-sppy is fully compatible with this narrower return type annotation.
temoa/extensions/single_vector_mga/output_summary.py (1)
18-62: Fix divide-by-zero and “0.0 means missing” delta logic.
orig_cost == 0will crash; andall((orig, option))suppresses legitimate deltas whenever either side is0.0.Proposed tweak
- conn = sqlite3.connect(config.output_database) - records: list[list[Any]] = [['Category', 'Label', 'Original', 'Option', 'Delta [%]']] - total_delta = (option_cost - orig_cost) / orig_cost * 100 - records.append(['Cost', 'Total Cost', orig_cost, option_cost, total_delta]) + with sqlite3.connect(config.output_database) as conn: + records: list[list[Any]] = [['Category', 'Label', 'Original', 'Option', 'Delta [%]']] + total_delta = ((option_cost - orig_cost) / orig_cost * 100) if orig_cost != 0 else None + records.append(['Cost', 'Total Cost', orig_cost, option_cost, total_delta]) @@ - for item in sorted(cast('Iterable[Any]', emission_labels)): + for item in sorted(cast(Iterable[Any], emission_labels)): orig = poll_emission( conn, scenarios[0], item, ) option = poll_emission(conn, scenarios[1], item) - row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None - records.append(['Emission', item, orig, option, row_delta]) + row_delta = ((option - orig) / orig * 100) if orig != 0 else None + records.append(['Emission', item, orig, option, row_delta]) @@ - conn.close() + # connection closed by context managertemoa/extensions/get_comm_tech.py (2)
12-40: Replace f-string SQL with parameterized query (and use=notIS).Even if the DB is “local”, treating it as untrusted avoids easy footguns.
Proposed fix
- cur.execute( - f"SELECT DISTINCT period FROM output_flow_out WHERE scenario is '{y}'" - ) + cur.execute( + 'SELECT DISTINCT period FROM output_flow_out WHERE scenario = ?', + (y,), + )
263-324: Fix incorrect error message variable + narrowget_inforeturn type (avoidAny).Line 311 currently formats
{file_ty}(a match object / None), not the user-provided filename.Proposed tweak
-def get_info(inputs: dict[str, str]) -> Any: +def get_info( + inputs: dict[str, str], +) -> OrderedDict[str, str] | dict[str, str] | dict[str, list[int]]: @@ - if not file_ty: - raise Exception(f'The file type {file_ty} is not recognized.') + if not file_ty: + raise Exception(f'The file type {inp_file} is not recognized.')temoa/extensions/modeling_to_generate_alternatives/worker.py (3)
35-60: Add__init__return type and keepself.opttyped consistently.Proposed tweak
def __init__( @@ - solver_log_path: Path | None = None, - ): + solver_log_path: Path | None = None, + ) -> None: @@ - self.opt = None # Initialize in run() + self.opt: Any = None # Initialize in run()
76-105: Fix solver log filename off-by-one (first solve currently writes..._0.log).One possible fix
- if self.solver_log_path: + if self.solver_log_path: # add the solver log path to options, if one is provided log_location = Path( self.solver_log_path, - f'solver_log_{str(self.worker_number)}_{self.solve_count}.log', + f'solver_log_{self.worker_number}_{self.solve_count + 1}.log', ) log_location_str = str(log_location)
103-138: HandleNoneresults explicitly and apply missing appsi_highs config setup.The code relies on catching
AttributeErrorto handleNoneresults, but should branch explicitly before callingcheck_optimal_termination(solve_res). This is clearer and more robust than catching exceptions.Additionally, unlike
mc_worker.py, this worker lacks the appsi-specific config setup for appsi_highs. After settingself.opt.options = self.solver_optionson line 93, add the config assignment loop:self.opt.options = self.solver_options + if self.solver_name.startswith('appsi_'): + for k, v in self.solver_options.items(): + try: + setattr(self.opt.config, k, v) + except (ValueError, AttributeError): + passWithout this, appsi_highs solver options won't be applied (appsi solvers primarily use
self.opt.config, not theoptionsattribute).For the None handling, add an explicit check before the try block at line 120:
toc = datetime.now() + if solve_res is None: + continue try:pyproject.toml (1)
86-92: Ruff excludes all oftemoa/extensions/while mypy checks most extensions with strict typing enabled — causing a linting gap.Mypy is configured to check all extensions except
breakeven(method_of_morris, modeling_to_generate_alternatives, monte_carlo, myopic, single_vector_mga, stochastics) with strict type checking. However, ruff excludes the entiretemoa/extensions/directory. This inconsistency means potential issues like unused imports, naming violations, and other style problems go undetected in the typed extensions. The presence of per-file-ignores for stochastics (line 120) further suggests these modules were intended to be linted.temoa/extensions/modeling_to_generate_alternatives/hull.py (3)
79-108: Uselogger.exception(...)once; avoid double-logging and keep stack traces.
The currentlogger.error(...); logger.error(e)loses traceback context and is noisier than needed.Proposed fix
try: @@ - except Exception as e: + except Exception as e: # scipy.spatial._qhull.QhullError may not be easily found by mypy - logger.error( - 'Attempt at hull construction from basis vectors failed.' - '\nMay be non-recoverable. Possibly try a set of random vectors to initialize the ' - 'Hull.' - ) - logger.error(e) - raise RuntimeError('Hull construction from vectors failed. See log file') from e + logger.exception( + "Attempt at hull construction from basis vectors failed. " + "May be non-recoverable; possibly try random vectors to initialize the Hull." + ) + raise RuntimeError("Hull construction from vectors failed") from e
119-131: Wrong-dimension points should raise immediately (don’t proceed to append).
As written, you log the error but still append; that can corrupt the hull state or crash onvstack.Proposed fix
def add_point(self, point: np.ndarray) -> None: if len(point) != self.dim: - logger.error( + logger.error( 'Tried adding a point to hull (dim: %d) with wrong dimensions %d. Point: %s', self.dim, len(point), point, ) + raise ValueError(f"Point dimension {len(point)} does not match hull dimension {self.dim}") if self.all_points is None: self.all_points = np.atleast_2d(point) else: self.all_points = np.vstack((self.all_points, point))
146-153: Return a consistent 2D empty array fromget_all_norms().
np.array([])is shape(0,), which is easy to misuse; returning(0, self.dim)is safer.Proposed fix
def get_all_norms(self) -> np.ndarray: @@ - return np.array([]) + return np.empty((0, self.dim))temoa/extensions/single_vector_mga/sv_mga_sequencer.py (2)
182-200: Tightenreitvotyping (it’s always length-6 here) or add a defensive check.
r, _, i, t, v, o = reitvowill throw if config/model feeds unexpected shapes.Proposed fix
def flow_idxs_from_eac_idx( - model: TemoaModel, reitvo: tuple[Any, ...] + model: TemoaModel, reitvo: tuple[Any, Any, Any, Any, Any, Any] ) -> tuple[list[tuple[Any, ...]], list[tuple[Any, ...]]]: @@ - r, _, i, t, v, o = reitvo + r, _, i, t, v, o = reitvo
203-209:verboseparameter is dead; either remove it or honor it (and make it keyword-only).
Today it’s a public API arg that does nothing, andprint(...)happens unconditionally.Proposed fix (keyword-only + gate prints)
def construct_obj( model: TemoaModel, emission_labels: Iterable[str], capacity_labels: Iterable[str], activity_labels: Iterable[str], - verbose: bool = True, + *, + verbose: bool = True, ) -> Expression | int: @@ - logger.warning(msg) - print(msg) + logger.warning(msg) + if verbose: + print(msg)Also applies to: 233-245
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (2)
266-290: Guard againstsum(coeffs) == 0when normalizing objective coefficients.
Hull normals (and especially mixed-sign vectors) can sum to ~0, producing inf/NaN coefficients and unstable solves.Proposed fix
coeffs = np.array(coeffs_list) - coeffs /= np.sum(coeffs) # normalize + denom = float(np.sum(coeffs)) + if np.isclose(denom, 0.0): + denom = float(np.sum(np.abs(coeffs))) + if np.isclose(denom, 0.0): + logger.warning("Cannot normalize coefficient vector (zero denominator); skipping") + return None + coeffs /= denom
331-360: Guard_generate_basis_coefficients()against categories with zero active variables (divide-by-zero).
If a category’s technologies have no entries intechnology_size, you’ll build an all-zero vector and normalize by 0.Proposed fix
for selected_cat in category_mapping: res = [] if selected_cat == default_cat: continue for cat in category_mapping: num_marks = sum(technology_size[tech] for tech in category_mapping[cat]) if cat == selected_cat: @@ res.extend(marks) + if not res: + continue entry = np.array(res) - entry = entry / np.array(np.sum(entry)) + s = float(np.sum(entry)) + if np.isclose(s, 0.0): + continue + entry = entry / s q.put(entry) # high value q.put(-entry) # low value
🤖 Fix all issues with AI agents
In @pyproject.toml:
- Around line 143-145: The mypy exclude regex in pyproject.toml only matches
paths with a trailing slash for the breakeven directory; update the exclude
entry (the string assigned to exclude) so the ^temoa/extensions/breakeven part
allows either a trailing slash or end-of-string (i.e., make the trailing slash
optional using a group like a non-capturing (?:/|$) after breakeven) so both
breakeven and breakeven/ are excluded.
In @temoa/extensions/get_comm_tech.py:
- Around line 12-19: The function get_tperiods uses generic Exception for input
validation; replace those raises with ValueError (or a module-specific custom
exception) to clearly signal invalid arguments: update the two raises in
get_tperiods (the "file type not recognized" and the "Please specify a
database..." cases) to raise ValueError (or a defined custom exception class)
and apply the same change to the similar checks at the other location referenced
(the block around lines 47-55) so argument errors consistently use the
appropriate exception type.
- Around line 74-76: Change the get_comm signature to require db_dat as a
keyword-only argument (e.g., def get_comm(inp_f: str, *, db_dat: bool) ->
OrderedDict[str, str]) so callers must use db_dat=...; apply the same change to
the other functions in this file that accept db_dat (the ones around lines
142-145) and update any local callers to pass db_dat by name rather than
position. Ensure only the signature is changed (keep type hints and return type)
and run tests to catch any missed call sites.
- Around line 203-210: In is_db_overwritten, replace the broad except Exception
around sqlite3.connect(db_file) with explicit exception handling for
sqlite3.Error (and optionally OSError) so only DB/connect related errors are
caught; open the connection in a try block and catch sqlite3.Error (and/or
OSError) to return False, and ensure the connection is closed or use a context
manager for the subsequent DB usage tied to the sqlite3.connect call.
In @temoa/extensions/method_of_morris/morris_evaluate.py:
- Around line 56-68: The debug log currently appends the literal template in
setting_entry instead of a formatted message; update the loop so that you build
the formatted message using the actual values (e.g., use param_info,
mm_sample[j], set_idx_tuple and the loop index j) and append that formatted
string to log_entry before calling logger.debug (replace the unformatted
setting_entry with a formatted string built via f-string or .format or %
formatting and ensure set_idx_tuple is rendered as a string or joined indices).
In @temoa/extensions/method_of_morris/morris_sequencer.py:
- Around line 351-352: The __del__ method currently calls self.con.close()
directly which can raise during garbage collection; wrap the close operation in
a try/except that catches sqlite3.Error (and optionally Exception) to
suppress/log errors, or remove __del__ and implement an explicit close method
(e.g., close or shutdown) that calls self.con.close() with proper error handling
and call that where the object is managed; update the class to use the new close
method and ensure any callers/integration points call it instead of relying on
__del__.
- Around line 5-31: Remove the unnecessary "# type: ignore[import-untyped]"
comments from the SALib imports (the lines importing morris, sample,
compute_groups_matrix, read_param_file) so their inline type hints are used, and
replace the joblib import annotation strategy: either install and rely on the
community "joblib-stubs" package so you can remove "# type:
ignore[import-untyped]" from the "from joblib import Parallel, delayed" import,
or explicitly annotate uses of joblib with types from those stubs; additionally,
replace broad Any return/type annotations in functions that consume SALib
outputs (e.g., functions referencing evaluate, sample, morris results) with
concrete types such as numpy.typing.NDArray and Dict[str, numpy.typing.NDArray]
to satisfy stricter linters.
In @temoa/extensions/method_of_morris/morris.py:
- Around line 24-26: The evaluate function currently relies on module globals
(config and db_file) created under resources.as_file and takes an unused
parameter k; change evaluate(param_names, param_values, data, k) to accept
explicit dependencies (e.g., add parameters config and db_path:
evaluate(param_names, param_values, data, k, *, config, db_path)) and stop using
the global db_file path so it uses the stable db_path string passed in; remove
or mark k as unused (or drop it) to fix the ARG001 warning. Then update all call
sites using delayed(evaluate)(...) to pass the new config= and db_path=
arguments (ensuring db_path is a persistent string, not a Path from a
short-lived resources.as_file context) and remove any reliance on module-level
config/db_file.
In @temoa/extensions/modeling_to_generate_alternatives/hull.py:
- Line 28: The __init__ signature in the Hull class currently accepts an unused
**kwargs: Any which violates lint/type rules; remove the **kwargs parameter from
the __init__ declarations (both occurrences referenced) so the constructors
become def __init__(self, points: np.ndarray) -> None: and update any docstrings
or type hints if present, ensuring all call sites remain unchanged since they
already pass only points.
In @temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py:
- Around line 358-360: The return statement using a backslash continuation is
fragile; replace it with a single parenthesized expression so it reads as:
return (status == pyo.TerminationCondition.optimal or str(status) ==
'convergenceCriteriaSatisfied'), ensuring the same logic and avoiding
trailing-whitespace issues—update the return that compares to
pyo.TerminationCondition.optimal and the 'convergenceCriteriaSatisfied' string
accordingly.
In
@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py:
- Around line 147-149: group_variable_names currently ignores the tech parameter
and returns category_mapping keys; change it to return the variable names for
the given tech from variable_index_mapping (i.e., the keys of
variable_index_mapping[tech]) and handle missing tech safely (return an empty
list if tech not present). Update the implementation of group_variable_names to
look up variable_index_mapping for the provided tech and return its variable
name keys rather than using category_mapping.
In @temoa/extensions/modeling_to_generate_alternatives/vector_manager.py:
- Around line 73-76: The abstract method finalize_tracker currently contains a
silent pass which can hide implementation omissions; replace the pass with
raising NotImplementedError (e.g., raise NotImplementedError("finalize_tracker
must be implemented by subclasses")) in the finalize_tracker method of the
abstract base so callers get a clear error, or alternatively remove the
@abstractmethod decorator if you intend it to be optional/implemented in the
base class.
- Around line 70-76: Replace the Any return on the abstract base with a generic
TypeVar: add TResult = TypeVar("TResult") and make the class Generic[TResult]
(e.g., class VectorManager(ABC, Generic[TResult])) and change
process_results(self, model: TemoaModel) -> TResult to use that TypeVar; update
subclasses such as TechActivityVectorManager to declare the concrete result type
(e.g., VectorManager[list[float]] or similar) so implementations keep their
list[float] signature and Ruff ANN401 is satisfied.
In @temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py:
- Around line 14-17: The SQL uses f-string interpolation which can cause SQL
injection and quoting issues; change the cur.execute call to use a parameterized
query (use "SELECT total_system_cost FROM output_objective WHERE scenario LIKE
?") and pass a single parameter like f"{scenario_name}-%" as the second argument
to cur.execute, then keep building obj_values_tuple from the fetched rows;
reference cur.execute, scenario_name, obj_values_tuple and the output_objective
table when making the change.
- Around line 17-20: The code builds obj_values_tuple and calls plt.hist with
bins=int(sqrt(len(obj_values_tuple))) which will raise if obj_values_tuple is
empty; update the plotting block to handle empty result sets by checking
len(obj_values_tuple) (or len(obj_values)) before calling plt.hist, and either
skip plotting (no-op) or use a safe fallback (e.g., bins=1) and/or log a
warning; make this change around the obj_values_tuple creation and the plt.hist
call so the empty-case is handled before invoking plt.show().
- Line 6: The code currently imports and/or constructs sqlite3.Connection
directly (symbol: Connection); replace that with the sqlite3.connect factory:
remove "from sqlite3 import Connection", import the sqlite3 module (or reference
sqlite3) and change any direct instantiation of Connection(...) to
sqlite3.connect(db_path, ...) so connections are created via sqlite3.connect
instead of direct construction (also update the other occurrence noted around
line 12).
In @temoa/extensions/monte_carlo/mc_sequencer.py:
- Line 55: The __init__ method in the class (def __init__(self, config:
TemoaConfig)) is missing an explicit return type; update the signature to
include -> None to satisfy Ruff and typing conventions (i.e., change def
__init__(self, config: TemoaConfig) to def __init__(self, config: TemoaConfig)
-> None) and run type/flake checks to confirm the warning is resolved.
In @temoa/extensions/myopic/myopic_progress_mapper.py:
- Around line 52-55: The report method currently accepts a plain str and
performs a runtime validation; change the signature of
MyopicProgressMapper.report to use
typing.Literal['load','solve','report','check'] for the status parameter so
invalid states are caught by mypy, add the necessary import for Literal (from
typing import Literal), and remove the redundant runtime ValueError check (or
keep it as an assert if you want a runtime guard) in the
MyopicProgressMapper.report implementation.
In @temoa/extensions/myopic/myopic_sequencer.py:
- Around line 66-77: Class attributes output_con, cursor, table_writer,
view_depth, step_size (and similar members declared non-optional) can be left
unset when config is None; change their declarations to optional (e.g., append |
None) and initialize them to None in the class or __init__, or alternatively
ensure they are always set in __init__ when config is None by providing sensible
defaults; update types for instance_queue/progress_mapper/config if needed to
accurately reflect possible None values and adjust any uses to handle None
safely (refer to output_con, cursor, table_writer, view_depth, step_size,
instance_queue, progress_mapper, config, and __init__).
In @temoa/extensions/single_vector_mga/output_summary.py:
- Around line 4-15: The code currently uses a string-based cast like
cast('Iterable[Any]', ...) and only imports Iterable under TYPE_CHECKING;
instead import Iterable directly from collections.abc at top-level and change
the cast to use the real type (cast(Iterable[Any], ...)) so you avoid string
type names; since from __future__ import annotations is enabled, a runtime
import of Iterable is fine—update the imports (remove Iterable from the
TYPE_CHECKING block) and replace any cast('Iterable[Any]', ...) occurrences with
cast(Iterable[Any], ...).
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 43-47: The ImportError is logged but the original exception
message is dropped when re-raising; update the exception raised in the
try/except around importing from mpisppy.opt.ef (where ExtensiveForm is imported
and logger.exception is called) to include the original ImportError message
(e.g., incorporate the caught exception 'e' into the RuntimeError message) and
still use "raise ... from e" to preserve the traceback.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
pyproject.tomltemoa/extensions/get_comm_tech.pytemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/method_of_morris/morris_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/hull.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/example_builds/scenario_analyzer.pytemoa/extensions/monte_carlo/mc_run.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/myopic/myopic_index.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.py
🧰 Additional context used
🧠 Learnings (4)
📓 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-11-03T22:55:02.519Z
Learnt from: ParticularlyPythonicBS
Repo: TemoaProject/temoa PR: 186
File: temoa/_internal/temoa_sequencer.py:60-63
Timestamp: 2025-11-03T22:55:02.519Z
Learning: In `TemoaSequencer.__init__`, when a `mode_override` is provided, it must be written back to `self.config.scenario_mode` (not just stored in `self.temoa_mode`) because `HybridLoader.create_data_dict()` has strict consistency checks that validate `config.scenario_mode` matches the presence/absence of `myopic_index`. Without this synchronization, MYOPIC mode would fail with a RuntimeError when the loader is instantiated.
Applied to files:
temoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py
📚 Learning: 2025-11-03T13:34:35.907Z
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.
Applied to files:
temoa/extensions/myopic/myopic_sequencer.py
📚 Learning: 2025-11-03T13:34:35.907Z
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.
Applied to files:
temoa/extensions/myopic/myopic_sequencer.py
🧬 Code graph analysis (12)
temoa/extensions/method_of_morris/morris_sequencer.py (1)
temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/myopic/myopic_progress_mapper.py (1)
temoa/extensions/myopic/myopic_index.py (1)
MyopicIndex(9-29)
temoa/extensions/method_of_morris/morris_evaluate.py (2)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/method_of_morris/morris.py (1)
evaluate(24-63)
temoa/extensions/monte_carlo/mc_worker.py (1)
temoa/_internal/data_brick.py (2)
DataBrick(23-83)data_brick_factory(86-117)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (2)
temoa/core/model.py (1)
TemoaModel(93-1137)temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (4)
group_members(243-244)group_variable_names(147-148)process_results(206-234)finalize_tracker(374-381)
temoa/extensions/modeling_to_generate_alternatives/manager_factory.py (4)
temoa/extensions/modeling_to_generate_alternatives/mga_constants.py (2)
MgaAxis(7-11)MgaWeighting(15-17)temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (1)
TechActivityVectorManager(46-381)temoa/core/model.py (1)
TemoaModel(93-1137)temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (1)
VectorManager(16-76)
temoa/extensions/stochastics/scenario_creator.py (3)
temoa/_internal/run_actions.py (1)
build_instance(121-155)temoa/data_io/hybrid_loader.py (1)
HybridLoader(70-930)temoa/data_io/loader_manifest.py (1)
LoadItem(26-59)
temoa/extensions/monte_carlo/mc_run.py (3)
temoa/data_io/hybrid_loader.py (1)
HybridLoader(70-930)temoa/core/config.py (1)
TemoaConfig(32-344)tests/test_mc_run.py (1)
tweak_factory(7-11)
temoa/extensions/method_of_morris/morris.py (3)
temoa/_internal/table_writer.py (2)
TableWriter(74-774)close(120-128)temoa/core/config.py (2)
TemoaConfig(32-344)build_config(201-253)temoa/extensions/method_of_morris/morris_evaluate.py (1)
evaluate(38-111)
temoa/extensions/stochastics/stochastic_sequencer.py (2)
temoa/extensions/stochastics/stochastic_config.py (1)
StochasticConfig(31-92)temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/single_vector_mga/output_summary.py (1)
temoa/core/config.py (1)
TemoaConfig(32-344)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (2)
temoa/extensions/modeling_to_generate_alternatives/hull.py (2)
Hull(15-169)update(79-117)temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (8)
VectorManager(16-76)expired(47-52)group_variable_names(55-57)random_input_vector_model(60-62)process_results(70-71)groups(36-38)group_members(41-43)finalize_tracker(74-76)
🪛 Ruff (0.14.10)
temoa/extensions/monte_carlo/mc_sequencer.py
55-55: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
temoa/extensions/get_comm_tech.py
16-16: Create your own exception
(TRY002)
16-16: Avoid specifying long messages outside the exception class
(TRY003)
19-19: Create your own exception
(TRY002)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
35-35: Possible SQL injection vector through string-based query construction
(S608)
51-51: Create your own exception
(TRY002)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Create your own exception
(TRY002)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Boolean-typed positional argument in function definition
(FBT001)
142-142: Boolean-typed positional argument in function definition
(FBT001)
209-209: Do not catch blind exception: Exception
(BLE001)
263-263: Dynamically typed expressions (typing.Any) are disallowed in get_info
(ANN401)
temoa/extensions/method_of_morris/morris_sequencer.py
138-138: Dynamically typed expressions (typing.Any) are disallowed in start
(ANN401)
204-204: Dynamically typed expressions (typing.Any) are disallowed in mm_samples
(ANN401)
205-205: Dynamically typed expressions (typing.Any) are disallowed in process_results
(ANN401)
temoa/extensions/modeling_to_generate_alternatives/hull.py
28-28: Unused method argument: kwargs
(ARG002)
28-28: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
101-105: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
106-106: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/method_of_morris/morris_evaluate.py
23-23: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
38-38: Dynamically typed expressions (typing.Any) are disallowed in mm_sample
(ANN401)
39-39: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
61-61: Avoid specifying long messages outside the exception class
(TRY003)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
100-103: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/monte_carlo/mc_worker.py
46-46: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
54-54: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
159-159: datetime.datetime.now() called without a tz argument
(DTZ005)
temoa/extensions/modeling_to_generate_alternatives/vector_manager.py
70-70: Dynamically typed expressions (typing.Any) are disallowed in process_results
(ANN401)
temoa/extensions/modeling_to_generate_alternatives/manager_factory.py
24-24: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/scenario_creator.py
21-21: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
21-21: Dynamically typed expressions (typing.Any) are disallowed in scenario_creator
(ANN401)
temoa/extensions/myopic/myopic_sequencer.py
97-97: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
560-560: Avoid specifying long messages outside the exception class
(TRY003)
574-574: Possible SQL injection vector through string-based query construction
(S608)
temoa/extensions/monte_carlo/mc_run.py
43-43: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
70-70: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
175-175: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
224-224: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
temoa/extensions/method_of_morris/morris.py
24-24: Dynamically typed expressions (typing.Any) are disallowed in param_values
(ANN401)
25-25: Unused function argument: k
(ARG001)
temoa/extensions/modeling_to_generate_alternatives/worker.py
35-35: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
117-117: datetime.datetime.now() called without a tz argument
(DTZ005)
temoa/extensions/single_vector_mga/sv_mga_sequencer.py
208-208: Boolean-typed positional argument in function definition
(FBT001)
208-208: Boolean default positional argument in function definition
(FBT002)
208-208: Unused static method argument: verbose
(ARG004)
temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py
147-147: Unused method argument: tech
(ARG002)
⏰ 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 (windows-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
| # Exclude specific directories from type checking will try to add them back gradually | ||
| exclude = "(?x)(^temoa/extensions/|^temoa/utilities/|^stubs/)" | ||
| exclude = "(?x)(^temoa/utilities/|^stubs/|^temoa/extensions/breakeven/)" | ||
|
|
There was a problem hiding this comment.
Tighten the mypy exclude regex to match breakeven both with and without trailing slash.
Current regex only matches ^temoa/extensions/breakeven/… paths.
Proposed tweak
-exclude = "(?x)(^temoa/utilities/|^stubs/|^temoa/extensions/breakeven/)"
+exclude = "(?x)(^temoa/utilities/|^stubs/|^temoa/extensions/breakeven(?:/|$))"🤖 Prompt for AI Agents
In @pyproject.toml around lines 143 - 145, The mypy exclude regex in
pyproject.toml only matches paths with a trailing slash for the breakeven
directory; update the exclude entry (the string assigned to exclude) so the
^temoa/extensions/breakeven part allows either a trailing slash or end-of-string
(i.e., make the trailing slash optional using a group like a non-capturing
(?:/|$) after breakeven) so both breakeven and breakeven/ are excluded.
| def get_tperiods(inp_f: str) -> dict[str, list[int]]: | ||
| file_ty = re.search(r'(\w+)\.(\w+)\b', inp_f) # Extract the input filename and extension | ||
|
|
||
| if not file_ty: | ||
| raise 'The file type %s is not recognized.' % inp_f | ||
| raise Exception(f'The file type {inp_f} is not recognized.') | ||
|
|
||
| elif file_ty.group(2) not in ('db', 'sqlite', 'sqlite3', 'sqlitedb'): | ||
| raise Exception('Please specify a database for finding scenarios') |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer ValueError/custom exceptions over generic Exception for argument validation.
Also applies to: 47-55
🧰 Tools
🪛 Ruff (0.14.10)
16-16: Create your own exception
(TRY002)
16-16: Avoid specifying long messages outside the exception class
(TRY003)
19-19: Create your own exception
(TRY002)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @temoa/extensions/get_comm_tech.py around lines 12 - 19, The function
get_tperiods uses generic Exception for input validation; replace those raises
with ValueError (or a module-specific custom exception) to clearly signal
invalid arguments: update the two raises in get_tperiods (the "file type not
recognized" and the "Please specify a database..." cases) to raise ValueError
(or a defined custom exception class) and apply the same change to the similar
checks at the other location referenced (the block around lines 47-55) so
argument errors consistently use the appropriate exception type.
| def get_comm(inp_f: str, db_dat: bool) -> OrderedDict[str, str]: | ||
| comm_list: dict[str, str] = {} | ||
| comm_set: set[str] = set() |
There was a problem hiding this comment.
Boolean positional params (db_dat: bool) are hard to read at call sites—prefer keyword-only usage.
Also applies to: 142-145
🧰 Tools
🪛 Ruff (0.14.10)
74-74: Boolean-typed positional argument in function definition
(FBT001)
🤖 Prompt for AI Agents
In @temoa/extensions/get_comm_tech.py around lines 74 - 76, Change the get_comm
signature to require db_dat as a keyword-only argument (e.g., def
get_comm(inp_f: str, *, db_dat: bool) -> OrderedDict[str, str]) so callers must
use db_dat=...; apply the same change to the other functions in this file that
accept db_dat (the ones around lines 142-145) and update any local callers to
pass db_dat by name rather than position. Ensure only the signature is changed
(keep type hints and return type) and run tests to catch any missed call sites.
| def evaluate(param_info: dict[int, list[Any]], mm_sample: Any, data: dict[str, Any], | ||
| i: int, config: TemoaConfig, log_queue: Any, log_level: int) -> list[float]: | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the morris_evaluate.py file structure
cat -n temoa/extensions/method_of_morris/morris_evaluate.py | head -100Repository: TemoaProject/temoa
Length of output: 4600
🏁 Script executed:
# Check the TableWriter and sqlite3 connection patterns
rg -n "def write_mm_results|write_mm_results\(" -A 5 temoa/ --type=pyRepository: TemoaProject/temoa
Length of output: 1191
🏁 Script executed:
# Search for sqlite3.connect calls in the codebase
rg -n "sqlite3\.connect\(" -B 2 -A 5 temoa/ --type=pyRepository: TemoaProject/temoa
Length of output: 23652
🏁 Script executed:
# Look for PRAGMA settings and busy_timeout configurations
rg -n "PRAGMA|busy_timeout|timeout" -B 2 -A 2 temoa/ --type=pyRepository: TemoaProject/temoa
Length of output: 11727
🏁 Script executed:
# Check table_writer.py specifically for database write handling
cat -n temoa/_internal/table_writer.pyRepository: TemoaProject/temoa
Length of output: 33661
Verify SQLite concurrency strategy for parallel workers—no timeouts/WAL configured.
Each worker calls TableWriter.write_mm_results(...) (which writes to output_database with PRAGMA foreign_keys = OFF only) and also opens its own sqlite3.connect(config.input_database) for reading. With multiple workers running in parallel, SQLite can hit "database is locked" errors unless you configure WAL mode (PRAGMA journal_mode = WAL) and/or set explicit busy timeouts (e.g., connection.execute('PRAGMA busy_timeout = 5000')). The default 5-second timeout on sqlite3.connect() may be insufficient under concurrent load.
Also applies to: 77-80
🧰 Tools
🪛 Ruff (0.14.10)
38-38: Dynamically typed expressions (typing.Any) are disallowed in mm_sample
(ANN401)
39-39: Dynamically typed expressions (typing.Any) are disallowed in log_queue
(ANN401)
| from __future__ import annotations | ||
|
|
||
| import sqlite3 | ||
| from sqlite3 import Connection | ||
| from typing import TYPE_CHECKING, Any, cast | ||
|
|
||
| import tabulate | ||
| if TYPE_CHECKING: | ||
| from collections.abc import Iterable | ||
|
|
||
| from temoa.core.config import TemoaConfig | ||
| from temoa.core.config import TemoaConfig | ||
|
|
||
|
|
||
| import tabulate # type: ignore[import-untyped] |
There was a problem hiding this comment.
Avoid string-based cast('Iterable[Any]', ...); import Iterable normally (cheaper + clearer).
With postponed annotations already enabled, a runtime import from collections.abc is fine and keeps cast() idiomatic.
Proposed tweak
-from typing import TYPE_CHECKING, Any, cast
+from collections.abc import Iterable
+from typing import TYPE_CHECKING, Any, cast
@@
- for item in sorted(cast('Iterable[Any]', emission_labels)):
+ for item in sorted(cast(Iterable[Any], emission_labels)):
@@
- for item in sorted(cast('Iterable[Any]', activity_labels)):
+ for item in sorted(cast(Iterable[Any], activity_labels)):
@@
- for item in sorted(cast('Iterable[Any]', capacity_labels)):
+ for item in sorted(cast(Iterable[Any], capacity_labels)):🤖 Prompt for AI Agents
In @temoa/extensions/single_vector_mga/output_summary.py around lines 4 - 15,
The code currently uses a string-based cast like cast('Iterable[Any]', ...) and
only imports Iterable under TYPE_CHECKING; instead import Iterable directly from
collections.abc at top-level and change the cast to use the real type
(cast(Iterable[Any], ...)) so you avoid string type names; since from __future__
import annotations is enabled, a runtime import of Iterable is fine—update the
imports (remove Iterable from the TYPE_CHECKING block) and replace any
cast('Iterable[Any]', ...) occurrences with cast(Iterable[Any], ...).
ab26865 to
e0d8c6b
Compare
ParticularlyPythonicBS
force-pushed
the
typing/extensions
branch
2 times, most recently
from
4e07581 to
f42a53f
Compare
ParticularlyPythonicBS
changed the title
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 (8)
temoa/extensions/stochastics/scenario_creator.py (1)
21-30: 🧹 Nitpick | 🔵 TrivialConsider a more specific return type.
While
Anyis pragmatic for mpi-sppy integration, the function returns aTemoaModelinstance (viabuild_instance). A more precise return type would improve type-safety for downstream callers.That said, since mpi-sppy's
ExtensiveFormhandles the return value generically,Anyis acceptable here.♻️ Optional improvement
+from temoa.temoa_model.temoa_model import TemoaModel + -def scenario_creator(scenario_name: str, **kwargs: Any) -> Any: +def scenario_creator(scenario_name: str, **kwargs: Any) -> TemoaModel:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/stochastics/scenario_creator.py` around lines 21 - 30, The function signature for scenario_creator currently returns Any; change it to the concrete model type returned by build_instance (e.g., -> TemoaModel) to improve type-safety: update the function annotation in scenario_creator to return TemoaModel, add the appropriate import for TemoaModel where build_instance is defined, and adjust any type hints in the file accordingly while keeping the same runtime behavior (leave Any only if mpi-sppy integration forces it).temoa/extensions/method_of_morris/morris_sequencer.py (1)
98-121: 🧹 Nitpick | 🔵 TrivialInconsistent cast patterns for config value extraction.
Line 90 uses
cast(str | float, pert)which is more precise, but lines 100, 110, and 121 still usecast('Any', ...). Consider using consistent, specific types throughout.♻️ Suggested consistency improvement
levels = morris_inputs.get('levels') if levels: - self.num_levels = int(cast('Any', levels)) + self.num_levels = int(cast(int | str, levels)) ... traj = morris_inputs.get('trajectories') if traj: - self.trajectories = int(cast('Any', traj)) + self.trajectories = int(cast(int | str, traj)) ... seed = morris_inputs.get('seed') - self.seed = int(cast('Any', seed)) if seed else None + self.seed = int(cast(int | str, seed)) if seed else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris_sequencer.py` around lines 98 - 121, The config parsing uses imprecise cast('Any', ...) for levels, trajectories, and seed; update those to specific union types consistent with the earlier pattern (e.g., use cast(str | float) or cast(str | int | float) as appropriate) so the subsequent int(...) conversions are type-safe and consistent—replace cast('Any', levels) in the assignment to self.num_levels, cast('Any', traj) in the assignment to self.trajectories, and cast('Any', seed) in the assignment to self.seed with explicit, consistent unions matching the actual input shapes used elsewhere (follow the earlier cast(str | float, pert) style).temoa/extensions/single_vector_mga/sv_mga_sequencer.py (1)
203-209:⚠️ Potential issue | 🟡 MinorRemove unused
verboseparameter or implement its functionality.The
verboseparameter (line 208) is declared but never used withinconstruct_obj. The docstring at line 215 references it, but the implementation doesn't use it for any conditional output.♻️ Option 1: Remove unused parameter
`@staticmethod` def construct_obj( model: TemoaModel, emission_labels: Iterable[str], capacity_labels: Iterable[str], activity_labels: Iterable[str], - verbose: bool = True, ) -> Expression | int: """ Construct an alternative OBJ statement from the config data Specifically, locate the labels passed in within the related variables and kluge together an objective to be minimized from them. - :param verbose: If True, report to console during construction... :param M: The basis model to search♻️ Option 2: Implement verbose logging
for label in emission_labels: idxs = [idx for idx in model.emission_activity if idx[1] == label] logger.debug('Located %d items for emission label: %s', len(idxs), label) + if verbose: + print(f'Located {len(idxs)} items for emission label: {label}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/single_vector_mga/sv_mga_sequencer.py` around lines 203 - 209, The verbose parameter on construct_obj (signature: construct_obj(model: TemoaModel, emission_labels: Iterable[str], capacity_labels: Iterable[str], activity_labels: Iterable[str], verbose: bool = True) -> Expression | int) is unused; either remove verbose from the function signature and update the function’s docstring and any callers to stop passing it, or implement its behavior by adding conditional logging/prints inside construct_obj (e.g., guard existing informational messages or add status logs) controlled by verbose; update the docstring so it accurately documents whether verbose exists and what it does, and ensure all references/call sites of construct_obj are adjusted accordingly.temoa/extensions/monte_carlo/mc_worker.py (1)
161-183:⚠️ Potential issue | 🟠 MajorNarrow the
AttributeErrorguard around solve-status handling.The
except AttributeError: passcurrently coversdata_brick_factory(model)and the logging path as well ascheck_optimal_termination(). If either one raises, the worker silently drops a completed solve and the sequencer never receives theDataBrick.💡 Narrow the guard
- try: - good_solve = check_optimal_termination(solve_res) - if good_solve: - data_brick = data_brick_factory(model) - self.results_queue.put(data_brick) - logger.info( - 'Worker %d solved a model in %0.2f minutes', - self.worker_number, - (toc - tic).total_seconds() / 60, - ) - if verbose: - print(f'Worker {self.worker_number} completed a successful solve') - else: - if solve_res is not None: - status = solve_res['Solver'].termination_condition - logger.info( - 'Worker %d did not solve. Results status: %s', - self.worker_number, - status, - ) - except AttributeError: - pass + if solve_res is None: + continue + + good_solve = check_optimal_termination(solve_res) + if good_solve: + data_brick = data_brick_factory(model) + self.results_queue.put(data_brick) + logger.info( + 'Worker %d solved a model in %0.2f minutes', + self.worker_number, + (toc - tic).total_seconds() / 60, + ) + if verbose: + print(f'Worker {self.worker_number} completed a successful solve') + else: + status = solve_res['Solver'].termination_condition + logger.info( + 'Worker %d did not solve. Results status: %s', + self.worker_number, + status, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/mc_worker.py` around lines 161 - 183, The current broad try/except around check_optimal_termination(solve_res) also covers data_brick_factory(model), self.results_queue.put, and the logging, causing successful solves to be silently dropped; narrow the guard so only the calls that can raise AttributeError due to a malformed solve_res are protected. Concretely, call check_optimal_termination(solve_res) inside a small try that catches AttributeError (and assign good_solve), separately handle the case where good_solve is True by calling data_brick_factory(model) and self.results_queue.put(...) outside that try, and likewise only guard the access to solve_res['Solver'].termination_condition with a minimal try/except; remove the broad except AttributeError: pass that currently wraps the whole block so exceptions from data_brick_factory, results_queue.put, or logging are not swallowed.temoa/extensions/modeling_to_generate_alternatives/worker.py (1)
119-138:⚠️ Potential issue | 🟠 MajorNarrow the
AttributeErrorcatch on the successful-solve path.The
except AttributeError: passon Lines 121-138 now also hides failures inresults_queue.put(model)and the status logging. If one of those raises, the worker silently discards a solved model and the MGA loop loses that iteration.💡 Narrow the guard
- try: - good_solve = check_optimal_termination(solve_res) - if good_solve: - self.results_queue.put(model) - logger.info( - 'Worker %d solved a model in %0.2f minutes', - self.worker_number, - (toc - tic).total_seconds() / 60, - ) - if verbose: - print(f'Worker {self.worker_number} completed a successful solve') - else: - if solve_res is not None: - status = solve_res['Solver'].termination_condition - logger.info( - 'Worker %d did not solve. Results status: %s', - self.worker_number, status - ) - except AttributeError: - pass + if solve_res is None: + continue + + good_solve = check_optimal_termination(solve_res) + if good_solve: + self.results_queue.put(model) + logger.info( + 'Worker %d solved a model in %0.2f minutes', + self.worker_number, + (toc - tic).total_seconds() / 60, + ) + if verbose: + print(f'Worker {self.worker_number} completed a successful solve') + else: + status = solve_res['Solver'].termination_condition + logger.info( + 'Worker %d did not solve. Results status: %s', + self.worker_number, + status, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/modeling_to_generate_alternatives/worker.py` around lines 119 - 138, The current broad "except AttributeError: pass" hides errors from results_queue.put(model) and logger.info; narrow the guard so only attribute-access on the solver result is caught. Wrap only the calls that can raise AttributeError from the solver object (the call to check_optimal_termination(solve_res) and the access solve_res['Solver'].termination_condition) in a small try/except that catches AttributeError and handles it (e.g., treat as not-good-solve or log), and remove the outer blanket except so exceptions from results_queue.put(model) and logger.info propagate normally; use the symbols check_optimal_termination, solve_res['Solver'].termination_condition, results_queue.put, and logger.info to locate the changes.temoa/extensions/method_of_morris/morris.py (1)
204-210:⚠️ Potential issue | 🟠 MajorWrap section labels in one-element list rows.
Lines 207 and 210 pass bare strings to
writer.writerow(). The csv module iterates over strings character-by-character, writing each character as a separate column. This expands the labels into many columns and corrupts the CSV structure.Minimal fix
- writer.writerow('Objective Function') + writer.writerow(['Objective Function']) @@ - writer.writerow('Cumulative CO2 Emissions') + writer.writerow(['Cumulative CO2 Emissions'])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris.py` around lines 204 - 210, The two CSV label writes pass bare strings to writer.writerow which causes the csv module to split the string into characters; update the writes in the with open('MMResults.csv', 'w') block so the label rows call writer.writerow with one-element lists (e.g., replace writer.writerow('Objective Function') and writer.writerow('Cumulative CO2 Emissions') with writer.writerow(['Objective Function']) and writer.writerow(['Cumulative CO2 Emissions'])), leaving the existing writer.writerow(unique_group_names), writer.writerow(line1), and writer.writerow(line2) intact.temoa/extensions/monte_carlo/mc_run.py (2)
170-185: 🧹 Nitpick | 🔵 TrivialWell-typed MCRun class with clear public API.
The class-level type hints and constructor signature are correctly defined. Consider adding
-> Noneto__init__per Ruff ANN204.♻️ Suggested fix
def __init__( self, scenario_name: str, run_index: int, data_store: dict[str, Any], included_tweaks: dict[Tweak, list[ChangeRecord]], - ): + ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/mc_run.py` around lines 170 - 185, The __init__ method of the MCRun class is missing a return type annotation; update the constructor signature of MCRun.__init__ to include an explicit -> None return type to satisfy type-checker/ruff (ANN204) and keep the public API type-safe.
38-54: 🧹 Nitpick | 🔵 TrivialWell-structured type annotations for the Tweak class.
The explicit class-level attribute declarations improve readability. Consider adding
-> Nonereturn type to__init__for completeness per Ruff ANN204.♻️ Suggested fix
- def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float): + def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/mc_run.py` around lines 38 - 54, The __init__ method on the Tweak class is missing an explicit return annotation; update the __init__ signature to include a -> None return type (i.e., def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float) -> None:) to satisfy Ruff ANN204 while leaving the body and validation logic (checks for indices tuple, adjustment in {'r','a','s'}, and value type) unchanged.
♻️ Duplicate comments (13)
temoa/extensions/stochastics/stochastic_sequencer.py (1)
43-47: 🧹 Nitpick | 🔵 TrivialType ignore comment is appropriate; consider including original error message.
The
# type: ignore[import-untyped]is correct since mpi-sppy lacks type stubs.However, the RuntimeError discards the original ImportError message. Including it would aid debugging (e.g., distinguishing "No module named 'mpisppy'" from submodule issues).
Suggested improvement
except ImportError as e: logger.exception('mpi-sppy is not installed. Please install it to use stochastic mode.') - raise RuntimeError('mpi-sppy not found') from e + raise RuntimeError(f'mpi-sppy not found: {e}') from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/stochastics/stochastic_sequencer.py` around lines 43 - 47, The except block that currently logs with logger.exception and raises RuntimeError('mpi-sppy not found') should preserve the original ImportError message; change the raise to include the original error details (e.g., raise RuntimeError(f"mpi-sppy not found: {e}") from e) and ensure the logger.exception call includes context if needed—update the exception handling around the import of ExtensiveForm in the try/except so the original ImportError message (variable e) is propagated.pyproject.toml (1)
147-147:⚠️ Potential issue | 🟡 MinorRegex may not exclude
temoa/extensions/breakevenwithout trailing slash.The pattern
^temoa/extensions/breakeven/only matches paths with a trailing slash. If mypy evaluates the directory path without a slash (e.g.,temoa/extensions/breakeven), it won't be excluded.Proposed fix
-exclude = "(?x)(^temoa/utilities/|^stubs/|^temoa/extensions/breakeven/)" +exclude = "(?x)(^temoa/utilities/|^stubs/|^temoa/extensions/breakeven(?:/|$))"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 147, The exclude regex currently uses `^temoa/extensions/breakeven/` which only matches when a trailing slash is present; change the pattern to also match the directory without a trailing slash by making the trailing slash optional (e.g., use `^temoa/extensions/breakeven(/|$)` or `^temoa/extensions/breakeven/?$`) so the `exclude` entry will skip both `temoa/extensions/breakeven` and `temoa/extensions/breakeven/`.temoa/extensions/get_comm_tech.py (2)
12-19: 🧹 Nitpick | 🔵 TrivialUse
ValueErrorinstead of genericExceptionfor input validation.Lines 16 and 19 raise generic
Exceptionfor argument validation errors.ValueErroris more appropriate and idiomatic for invalid input conditions.♻️ Proposed fix
if not file_ty: - raise Exception(f'The file type {inp_f} is not recognized.') + raise ValueError(f'The file type {inp_f} is not recognized.') elif file_ty.group(2) not in ('db', 'sqlite', 'sqlite3', 'sqlitedb'): - raise Exception('Please specify a database for finding scenarios') + raise ValueError('Please specify a database for finding scenarios')Also applies to lines 51, 54.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/get_comm_tech.py` around lines 12 - 19, In get_tperiods replace the generic Exception raises used for input validation with ValueError: change the raise in the file-type-check branch (the raise about unrecognized file type) and the raise when the extension is not a database to raise ValueError instead; apply the same replacement for the two other similar validation raises referenced at lines 51 and 54 (search for other raises in get_tperiods or nearby helper functions and switch them to ValueError) so invalid input errors are reported idiomatically.
207-210: 🧹 Nitpick | 🔵 TrivialCatch specific exception type instead of broad
Exception.The broad
except Exceptioncan mask unexpected errors. Sincesqlite3.connect()raisessqlite3.Errorsubclasses for database-related failures, catch that specifically.♻️ Proposed fix
try: con = sqlite3.connect(db_file) - except Exception: + except sqlite3.Error: return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/get_comm_tech.py` around lines 207 - 210, The broad except around sqlite3.connect(db_file) should be narrowed to database-specific exceptions: replace "except Exception" with "except sqlite3.Error as e" in the block that initializes con (the sqlite3.connect call) so only SQLite errors are caught; optionally surface or log the caught error (e) before returning False to preserve current behavior. Ensure the change is made in the same function where con is assigned after sqlite3.connect.temoa/extensions/myopic/myopic_progress_mapper.py (1)
52-55: 🧹 Nitpick | 🔵 TrivialConsider using
Literaltype forstatusparameter.Since Python 3.12+ is required and the valid values are fixed (
'load','solve','report','check'), usingLiteralwould catch invalid values at type-check time rather than runtime.♻️ Proposed enhancement
+from typing import Literal + class MyopicProgressMapper: - def report(self, mi: MyopicIndex, status: str) -> None: + def report( + self, + mi: MyopicIndex, + status: Literal['load', 'solve', 'report', 'check'], + ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/myopic/myopic_progress_mapper.py` around lines 52 - 55, Change the report method signature to use typing.Literal for the status parameter so invalid statuses are caught by static type checkers: import Literal from typing and update MyopicProgressMapper.report to accept status: Literal['load','solve','report','check'] (keep the runtime check removable or keep for safety); update any callers if they pass non-literal values or widen types accordingly so type-checking passes. Ensure the import and the updated signature reference the existing report method from MyopicProgressMapper and remove or relax the manual runtime set-membership check if you rely fully on the Literal typing.temoa/extensions/method_of_morris/morris_sequencer.py (1)
351-352:⚠️ Potential issue | 🟡 MinorWrap
con.close()in try/except to avoid exceptions during garbage collection.Raising exceptions from
__del__can cause "Exception ignored in:" warnings during GC. Consider wrapping the close call or using an explicitclose()method.♻️ Proposed fix
def __del__(self) -> None: - self.con.close() + try: + self.con.close() + except sqlite3.Error: + pass # Ignore errors during cleanup🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris_sequencer.py` around lines 351 - 352, The destructor __del__ currently calls self.con.close() directly which can raise during garbage collection; wrap the close in a try/except (catch Exception) to swallow or log errors, or implement an explicit close() method that performs the try/except close and have __del__ call that safe close routine; update references to use the new close() if needed and ensure the attribute self.con is checked (e.g., is not None) before attempting to close.temoa/extensions/single_vector_mga/output_summary.py (1)
31-52:⚠️ Potential issue | 🟡 MinorZero values incorrectly treated as missing data.
The
all((orig, option))check on lines 38, 43, and 50 treats0.0as falsy. Legitimate zero emissions/activity/capacity values will producerow_delta = Noneinstead of computing the actual percentage change (which would be division by zero fororig=0, but valid foroption=0with non-zeroorig).Consider checking explicitly for
Noneand handling the division-by-zero case:🐛 Proposed fix
- row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None + row_delta = (option - orig) / orig * 100 if orig != 0 else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/single_vector_mga/output_summary.py` around lines 31 - 52, The loops use all((orig, option)) which treats 0.0 as missing; change the checks to explicit None and divide-by-zero handling for poll_emission/poll_activity/poll_capacity results: compute row_delta only when orig is not None and option is not None; if orig == 0 and option == 0 set row_delta = 0.0; if orig == 0 and option != 0 set row_delta = None (or math.inf if you prefer to represent infinite percent change); otherwise compute row_delta = (option - orig) / orig * 100. Apply this fix to the three places that set row_delta in the Emission, Activity, and Capacity loops.temoa/extensions/single_vector_mga/sv_mga_sequencer.py (1)
125-134: 🧹 Nitpick | 🔵 TrivialConsider caching
svmga_inputsto avoid duplication.The pattern
svmga_inputs = self.config.svmga_inputs or {}is duplicated from line 52. Sinceself.configdoesn't change during the object's lifetime, consider storing the normalized dict in__init__.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/single_vector_mga/sv_mga_sequencer.py` around lines 125 - 134, Duplicate normalization of svmga_inputs should be moved into the class initializer: in __init__, read self.config.svmga_inputs or {} once (assign to a new instance attribute like self._svmga_inputs) and then update calls such as the one that passes emission_labels/capacity_labels/activity_labels to SvMgaSequencer.construct_obj to derive those lists from self._svmga_inputs instead of re-evaluating self.config.svmga_inputs; this centralizes the logic, avoids duplication, and preserves behavior.temoa/extensions/method_of_morris/morris_evaluate.py (1)
65-67:⚠️ Potential issue | 🟡 MinorLog format string is never interpolated with actual values.
The
setting_entryformat string contains placeholders (%d,%s,%s,%f) but is appended verbatim tolog_entrywithout being formatted. The debug log will output the raw format string instead of the actual parameter values.🐛 Proposed fix
- setting_entry = 'run # %d: Setting param %s[%s] to value: %f' - log_entry.append(setting_entry) + setting_entry = f'run # {i + 1}: Setting param {param_name}[{set_idx_tuple}] to value: {mm_sample[j]:.6f}' + log_entry.append(setting_entry)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris_evaluate.py` around lines 65 - 67, The log format string in setting_entry is appended verbatim to log_entry so placeholders are never substituted; update the code around setting_entry/log_entry/logger.debug to format the string with the actual values before appending (e.g., interpolate the integer, parameter name(s) and numeric value used in the loop that builds setting_entry) so log_entry contains the fully rendered message and logger.debug('\n '.join(log_entry)) emits the real values; search for the variables setting_entry and log_entry (and the surrounding loop or function where parameter name/index/value are available) and replace the raw format string append with a formatted string using those symbols.temoa/extensions/method_of_morris/morris.py (1)
24-25:⚠️ Potential issue | 🔴 CriticalMake
evaluate()acceptconfigand a stable DB path explicitly.
evaluate()still reads module-levelconfiganddb_file, butdb_fileis created inside theresources.as_file(...)scope on Lines 72-74 while theParallel(...)call happens later on Lines 155-156. Once that context exits, the extracted path is no longer guaranteed to exist, so worker solves can fail reopening the SQLite file. Pass both dependencies explicitly and keep the evaluation loop inside the lifetime-safe scope.Run the following script to confirm the scope/lifetime mismatch:
#!/bin/bash # Show that evaluate() reads module globals and that Parallel runs after the as_file scope. sed -n '24,63p' temoa/extensions/method_of_morris/morris.py printf '\n--- resources.as_file scope ---\n' sed -n '70,76p' temoa/extensions/method_of_morris/morris.py printf '\n--- Parallel call site ---\n' sed -n '154,157p' temoa/extensions/method_of_morris/morris.pyExpected result:
evaluate()referencesconfig/db_file,db_fileis defined underresources.as_file(...), and theParallel(...)call sits outside that scope.Also applies to: 49-49, 72-74, 155-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris.py` around lines 24 - 25, The evaluate function currently reads module-level config and db_file which can be torn down because db_file is created inside resources.as_file(...) — change evaluate(param_names, param_values, data, k) to accept the config and a stable DB path (e.g. db_path) as explicit parameters, update all call sites that invoke evaluate (including the Parallel(...) invocation) to pass the same config and the file path returned from resources.as_file, and ensure the Parallel call (and its worker lifetimes) remain inside the resources.as_file scope (or use a guaranteed-persistent temp path) so that worker processes reopen a valid SQLite file; update/remove any other references to module globals config/db_file inside evaluate and related helpers.temoa/extensions/myopic/myopic_sequencer.py (1)
66-78: 🛠️ Refactor suggestion | 🟠 MajorThe
config=Nonepath still leaves non-optional attributes unset.When
configisNone, Lines 85-113 skip initializingoutput_con,cursor,table_writer,view_depth, andstep_size, but Lines 72-76 still advertise them as always present. That keeps theconfig=Nonetest shunt working only through manual mutation and weakens the type surface this PR is adding.💡 Possible shape
- output_con: Connection - cursor: Cursor - table_writer: TableWriter - view_depth: int - step_size: int + output_con: Connection | None + cursor: Cursor | None + table_writer: TableWriter | None + view_depth: int | None + step_size: int | None def __init__(self, config: TemoaConfig | None) -> None: self.capacity_epsilon = 1e-5 self.debugging = False self.optimization_periods = None self.instance_queue = deque() # a LIFO queue self.progress_mapper = None self.config = config + self.output_con = None + self.cursor = None + self.table_writer = None + self.view_depth = None + self.step_size = NoneRun the following script to confirm the unset-attribute path and the test shunt:
#!/bin/bash sed -n '66,113p' temoa/extensions/myopic/myopic_sequencer.py printf '\n--- config=None test shunt ---\n' sed -n '35,48p' tests/test_myopic_sequencer.pyExpected result: the constructor skips initializing those fields when
config=None, and the test shunt compensates by mutating attributes after construction.Also applies to: 85-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/myopic/myopic_sequencer.py` around lines 66 - 78, The constructor permits config=None but leaves required attributes uninitialized; update the class so the attributes referenced in the annotation (output_con, cursor, table_writer, view_depth, step_size) are either given safe defaults in __init__ when config is None or their type annotations are changed to Optional[...] and they are explicitly set to None in the config=None branch. Concretely, modify __init__(self, config: TemoaConfig | None) to initialize self.output_con, self.cursor, self.table_writer, self.view_depth, and self.step_size for the config None path (or set them to None and adjust their annotated types at the class level) so code/tests no longer rely on post-construction mutation; refer to the attributes named output_con, cursor, table_writer, view_depth, and step_size to locate the fixes.temoa/extensions/monte_carlo/mc_run.py (1)
224-228:⚠️ Potential issue | 🟠 MajorAdd validation for
monte_carlo_inputsbefore accessing.The code directly accesses
self.config.monte_carlo_inputs['run_settings']without checking ifmonte_carlo_inputsisNoneor contains the'run_settings'key. Sincemonte_carlo_inputsis typed asdict[str, object] | NoneinTemoaConfig, this will raiseTypeErrorifNoneandKeyErrorif the key is missing. The# type: ignorecomment masks both issues.Also consider adding
-> Nonereturn type per Ruff ANN204.,
🛡️ Proposed fix
- def __init__(self, config: TemoaConfig, data_store: dict[str, Any]): + def __init__(self, config: TemoaConfig, data_store: dict[str, Any]) -> None: self.config = config self.data_store = data_store self.tweak_factory = TweakFactory(data_store) - self.settings_file = Path(self.config.monte_carlo_inputs['run_settings']) # type: ignore + if self.config.monte_carlo_inputs is None: + raise ValueError('monte_carlo_inputs must be configured for Monte Carlo runs') + if 'run_settings' not in self.config.monte_carlo_inputs: + raise ValueError("monte_carlo_inputs must contain 'run_settings' key") + self.settings_file = Path(str(self.config.monte_carlo_inputs['run_settings']))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/mc_run.py` around lines 224 - 228, In __init__ of the class (the __init__ method that takes TemoaConfig and data_store), validate that self.config.monte_carlo_inputs is not None and contains the 'run_settings' key before assigning self.settings_file; if the check fails raise a clear ValueError (or provide a safe default) rather than using # type: ignore, and then set self.settings_file = Path(self.config.monte_carlo_inputs['run_settings']); also add the explicit return type -> None on __init__ to satisfy ANN204.temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (1)
147-148:⚠️ Potential issue | 🟠 MajorThe
techparameter is unused; method returns all category keys instead of filtering.The implementation ignores the
techargument and returns allcategory_mappingkeys. Per the parent class contract and the docstring semantics (variable names for group members), this should likely return variable names for the specific technology. The data is available invariable_index_mapping.,
🔧 Proposed fix (if filtering by tech is intended)
def group_variable_names(self, tech: str) -> list[str]: - return [str(k) for k in self.category_mapping.keys()] + return list(self.variable_index_mapping.get(tech, {}).keys())🔧 Alternative fix (if tech is intentionally unused)
If the current behavior is intentional (returning all category names regardless of tech), rename the parameter to indicate non-use:
- def group_variable_names(self, tech: str) -> list[str]: + def group_variable_names(self, _tech: str) -> list[str]: return [str(k) for k in self.category_mapping.keys()]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py` around lines 147 - 148, The method group_variable_names currently ignores the tech parameter and returns all category_mapping keys; change it to return only variable names for the given tech by filtering category_mapping keys against the entries in self.variable_index_mapping for that tech (e.g., use self.variable_index_mapping.get(tech, []) and include only keys present in that list/set), and return the filtered list; if the original behavior was intentional, instead remove or rename the tech parameter to indicate it's unused.
🤖 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/extensions/get_comm_tech.py`:
- Around line 34-36: Replace the f-string SQL interpolation in the cur.execute
call that queries output_flow_out by using a parameterized query instead: stop
embedding the variable y directly into the SQL string and pass it as a query
parameter to cur.execute (referencing the cur.execute call, the variable y, and
table output_flow_out) so it matches the parameterized pattern used elsewhere in
this file and avoids string interpolation.
In `@temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py`:
- Around line 376-377: The destructor __del__ unconditionally calls
self.con.close() which can raise AttributeError if __init__ failed before
setting self.con; change __del__ to safely retrieve the connection (e.g. con =
getattr(self, "con", None)) and only call con.close() if con is not None
(optionally wrap close in try/except to ignore errors during interpreter
shutdown) so partially constructed instances don't raise during garbage
collection; update the method on the same class that defines __init__ and
self.con.
In `@temoa/extensions/monte_carlo/mc_run.py`:
- Around line 68-77: The __init__ method lacks an explicit return annotation;
update the constructor signature of the class containing val_data to include "->
None" (i.e., def __init__(self, data_store: dict[str, Any]) -> None:) so it
satisfies Ruff ANN204 while preserving the existing TypeError guard and
assignment to self.val_data; ensure no other behavior changes.
In `@temoa/extensions/monte_carlo/mc_worker.py`:
- Around line 46-61: The constructor for MCWorker uses **kwargs and then
accesses kwargs['solver_name'] and kwargs['solver_options'], which hides
required args and risks KeyError; change MCWorker.__init__ to accept explicit
parameters solver_name: str and solver_options: dict[str, Any] (remove
**kwargs), assign them to self.solver_name and self.solver_options in the body,
update any call sites that unpacked a dict into **kwargs to pass solver_name=...
and solver_options=... directly, and adjust type annotations for these
parameters to match the class attribute types.
---
Outside diff comments:
In `@temoa/extensions/method_of_morris/morris_sequencer.py`:
- Around line 98-121: The config parsing uses imprecise cast('Any', ...) for
levels, trajectories, and seed; update those to specific union types consistent
with the earlier pattern (e.g., use cast(str | float) or cast(str | int | float)
as appropriate) so the subsequent int(...) conversions are type-safe and
consistent—replace cast('Any', levels) in the assignment to self.num_levels,
cast('Any', traj) in the assignment to self.trajectories, and cast('Any', seed)
in the assignment to self.seed with explicit, consistent unions matching the
actual input shapes used elsewhere (follow the earlier cast(str | float, pert)
style).
In `@temoa/extensions/method_of_morris/morris.py`:
- Around line 204-210: The two CSV label writes pass bare strings to
writer.writerow which causes the csv module to split the string into characters;
update the writes in the with open('MMResults.csv', 'w') block so the label rows
call writer.writerow with one-element lists (e.g., replace
writer.writerow('Objective Function') and writer.writerow('Cumulative CO2
Emissions') with writer.writerow(['Objective Function']) and
writer.writerow(['Cumulative CO2 Emissions'])), leaving the existing
writer.writerow(unique_group_names), writer.writerow(line1), and
writer.writerow(line2) intact.
In `@temoa/extensions/modeling_to_generate_alternatives/worker.py`:
- Around line 119-138: The current broad "except AttributeError: pass" hides
errors from results_queue.put(model) and logger.info; narrow the guard so only
attribute-access on the solver result is caught. Wrap only the calls that can
raise AttributeError from the solver object (the call to
check_optimal_termination(solve_res) and the access
solve_res['Solver'].termination_condition) in a small try/except that catches
AttributeError and handles it (e.g., treat as not-good-solve or log), and remove
the outer blanket except so exceptions from results_queue.put(model) and
logger.info propagate normally; use the symbols check_optimal_termination,
solve_res['Solver'].termination_condition, results_queue.put, and logger.info to
locate the changes.
In `@temoa/extensions/monte_carlo/mc_run.py`:
- Around line 170-185: The __init__ method of the MCRun class is missing a
return type annotation; update the constructor signature of MCRun.__init__ to
include an explicit -> None return type to satisfy type-checker/ruff (ANN204)
and keep the public API type-safe.
- Around line 38-54: The __init__ method on the Tweak class is missing an
explicit return annotation; update the __init__ signature to include a -> None
return type (i.e., def __init__(self, param_name: str, indices: tuple[Any, ...],
adjustment: str, value: float) -> None:) to satisfy Ruff ANN204 while leaving
the body and validation logic (checks for indices tuple, adjustment in
{'r','a','s'}, and value type) unchanged.
In `@temoa/extensions/monte_carlo/mc_worker.py`:
- Around line 161-183: The current broad try/except around
check_optimal_termination(solve_res) also covers data_brick_factory(model),
self.results_queue.put, and the logging, causing successful solves to be
silently dropped; narrow the guard so only the calls that can raise
AttributeError due to a malformed solve_res are protected. Concretely, call
check_optimal_termination(solve_res) inside a small try that catches
AttributeError (and assign good_solve), separately handle the case where
good_solve is True by calling data_brick_factory(model) and
self.results_queue.put(...) outside that try, and likewise only guard the access
to solve_res['Solver'].termination_condition with a minimal try/except; remove
the broad except AttributeError: pass that currently wraps the whole block so
exceptions from data_brick_factory, results_queue.put, or logging are not
swallowed.
In `@temoa/extensions/single_vector_mga/sv_mga_sequencer.py`:
- Around line 203-209: The verbose parameter on construct_obj (signature:
construct_obj(model: TemoaModel, emission_labels: Iterable[str],
capacity_labels: Iterable[str], activity_labels: Iterable[str], verbose: bool =
True) -> Expression | int) is unused; either remove verbose from the function
signature and update the function’s docstring and any callers to stop passing
it, or implement its behavior by adding conditional logging/prints inside
construct_obj (e.g., guard existing informational messages or add status logs)
controlled by verbose; update the docstring so it accurately documents whether
verbose exists and what it does, and ensure all references/call sites of
construct_obj are adjusted accordingly.
In `@temoa/extensions/stochastics/scenario_creator.py`:
- Around line 21-30: The function signature for scenario_creator currently
returns Any; change it to the concrete model type returned by build_instance
(e.g., -> TemoaModel) to improve type-safety: update the function annotation in
scenario_creator to return TemoaModel, add the appropriate import for TemoaModel
where build_instance is defined, and adjust any type hints in the file
accordingly while keeping the same runtime behavior (leave Any only if mpi-sppy
integration forces it).
---
Duplicate comments:
In `@pyproject.toml`:
- Line 147: The exclude regex currently uses `^temoa/extensions/breakeven/`
which only matches when a trailing slash is present; change the pattern to also
match the directory without a trailing slash by making the trailing slash
optional (e.g., use `^temoa/extensions/breakeven(/|$)` or
`^temoa/extensions/breakeven/?$`) so the `exclude` entry will skip both
`temoa/extensions/breakeven` and `temoa/extensions/breakeven/`.
In `@temoa/extensions/get_comm_tech.py`:
- Around line 12-19: In get_tperiods replace the generic Exception raises used
for input validation with ValueError: change the raise in the file-type-check
branch (the raise about unrecognized file type) and the raise when the extension
is not a database to raise ValueError instead; apply the same replacement for
the two other similar validation raises referenced at lines 51 and 54 (search
for other raises in get_tperiods or nearby helper functions and switch them to
ValueError) so invalid input errors are reported idiomatically.
- Around line 207-210: The broad except around sqlite3.connect(db_file) should
be narrowed to database-specific exceptions: replace "except Exception" with
"except sqlite3.Error as e" in the block that initializes con (the
sqlite3.connect call) so only SQLite errors are caught; optionally surface or
log the caught error (e) before returning False to preserve current behavior.
Ensure the change is made in the same function where con is assigned after
sqlite3.connect.
In `@temoa/extensions/method_of_morris/morris_evaluate.py`:
- Around line 65-67: The log format string in setting_entry is appended verbatim
to log_entry so placeholders are never substituted; update the code around
setting_entry/log_entry/logger.debug to format the string with the actual values
before appending (e.g., interpolate the integer, parameter name(s) and numeric
value used in the loop that builds setting_entry) so log_entry contains the
fully rendered message and logger.debug('\n '.join(log_entry)) emits the real
values; search for the variables setting_entry and log_entry (and the
surrounding loop or function where parameter name/index/value are available) and
replace the raw format string append with a formatted string using those
symbols.
In `@temoa/extensions/method_of_morris/morris_sequencer.py`:
- Around line 351-352: The destructor __del__ currently calls self.con.close()
directly which can raise during garbage collection; wrap the close in a
try/except (catch Exception) to swallow or log errors, or implement an explicit
close() method that performs the try/except close and have __del__ call that
safe close routine; update references to use the new close() if needed and
ensure the attribute self.con is checked (e.g., is not None) before attempting
to close.
In `@temoa/extensions/method_of_morris/morris.py`:
- Around line 24-25: The evaluate function currently reads module-level config
and db_file which can be torn down because db_file is created inside
resources.as_file(...) — change evaluate(param_names, param_values, data, k) to
accept the config and a stable DB path (e.g. db_path) as explicit parameters,
update all call sites that invoke evaluate (including the Parallel(...)
invocation) to pass the same config and the file path returned from
resources.as_file, and ensure the Parallel call (and its worker lifetimes)
remain inside the resources.as_file scope (or use a guaranteed-persistent temp
path) so that worker processes reopen a valid SQLite file; update/remove any
other references to module globals config/db_file inside evaluate and related
helpers.
In
`@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py`:
- Around line 147-148: The method group_variable_names currently ignores the
tech parameter and returns all category_mapping keys; change it to return only
variable names for the given tech by filtering category_mapping keys against the
entries in self.variable_index_mapping for that tech (e.g., use
self.variable_index_mapping.get(tech, []) and include only keys present in that
list/set), and return the filtered list; if the original behavior was
intentional, instead remove or rename the tech parameter to indicate it's
unused.
In `@temoa/extensions/monte_carlo/mc_run.py`:
- Around line 224-228: In __init__ of the class (the __init__ method that takes
TemoaConfig and data_store), validate that self.config.monte_carlo_inputs is not
None and contains the 'run_settings' key before assigning self.settings_file; if
the check fails raise a clear ValueError (or provide a safe default) rather than
using # type: ignore, and then set self.settings_file =
Path(self.config.monte_carlo_inputs['run_settings']); also add the explicit
return type -> None on __init__ to satisfy ANN204.
In `@temoa/extensions/myopic/myopic_progress_mapper.py`:
- Around line 52-55: Change the report method signature to use typing.Literal
for the status parameter so invalid statuses are caught by static type checkers:
import Literal from typing and update MyopicProgressMapper.report to accept
status: Literal['load','solve','report','check'] (keep the runtime check
removable or keep for safety); update any callers if they pass non-literal
values or widen types accordingly so type-checking passes. Ensure the import and
the updated signature reference the existing report method from
MyopicProgressMapper and remove or relax the manual runtime set-membership check
if you rely fully on the Literal typing.
In `@temoa/extensions/myopic/myopic_sequencer.py`:
- Around line 66-78: The constructor permits config=None but leaves required
attributes uninitialized; update the class so the attributes referenced in the
annotation (output_con, cursor, table_writer, view_depth, step_size) are either
given safe defaults in __init__ when config is None or their type annotations
are changed to Optional[...] and they are explicitly set to None in the
config=None branch. Concretely, modify __init__(self, config: TemoaConfig |
None) to initialize self.output_con, self.cursor, self.table_writer,
self.view_depth, and self.step_size for the config None path (or set them to
None and adjust their annotated types at the class level) so code/tests no
longer rely on post-construction mutation; refer to the attributes named
output_con, cursor, table_writer, view_depth, and step_size to locate the fixes.
In `@temoa/extensions/single_vector_mga/output_summary.py`:
- Around line 31-52: The loops use all((orig, option)) which treats 0.0 as
missing; change the checks to explicit None and divide-by-zero handling for
poll_emission/poll_activity/poll_capacity results: compute row_delta only when
orig is not None and option is not None; if orig == 0 and option == 0 set
row_delta = 0.0; if orig == 0 and option != 0 set row_delta = None (or math.inf
if you prefer to represent infinite percent change); otherwise compute row_delta
= (option - orig) / orig * 100. Apply this fix to the three places that set
row_delta in the Emission, Activity, and Capacity loops.
In `@temoa/extensions/single_vector_mga/sv_mga_sequencer.py`:
- Around line 125-134: Duplicate normalization of svmga_inputs should be moved
into the class initializer: in __init__, read self.config.svmga_inputs or {}
once (assign to a new instance attribute like self._svmga_inputs) and then
update calls such as the one that passes
emission_labels/capacity_labels/activity_labels to SvMgaSequencer.construct_obj
to derive those lists from self._svmga_inputs instead of re-evaluating
self.config.svmga_inputs; this centralizes the logic, avoids duplication, and
preserves behavior.
In `@temoa/extensions/stochastics/stochastic_sequencer.py`:
- Around line 43-47: The except block that currently logs with logger.exception
and raises RuntimeError('mpi-sppy not found') should preserve the original
ImportError message; change the raise to include the original error details
(e.g., raise RuntimeError(f"mpi-sppy not found: {e}") from e) and ensure the
logger.exception call includes context if needed—update the exception handling
around the import of ExtensiveForm in the try/except so the original ImportError
message (variable e) is propagated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0373532f-74e4-4271-82a3-fcfc6e627527
📒 Files selected for processing (23)
pyproject.tomltemoa/extensions/get_comm_tech.pytemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/method_of_morris/morris_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/hull.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/example_builds/scenario_analyzer.pytemoa/extensions/monte_carlo/mc_run.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/myopic/myopic_index.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.py
| val_data: dict[str, Any] | ||
|
|
||
| def __init__(self, data_store: dict[str, Any]): | ||
| """ | ||
| make a new factor and use data_store as a validation tool | ||
| :param data_store: the data dictionary holding the base values for the model | ||
| """ | ||
| if not isinstance(data_store, dict): | ||
| raise TypeError('data_store must be a dict') | ||
| self.val_data = data_store |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good typing additions with proper validation.
The val_data attribute is correctly typed, and the type guard for data_store is retained. Consider adding -> None return type to __init__ per Ruff ANN204.
♻️ Suggested fix
- def __init__(self, data_store: dict[str, Any]):
+ def __init__(self, data_store: dict[str, Any]) -> None:📝 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.
| val_data: dict[str, Any] | |
| def __init__(self, data_store: dict[str, Any]): | |
| """ | |
| make a new factor and use data_store as a validation tool | |
| :param data_store: the data dictionary holding the base values for the model | |
| """ | |
| if not isinstance(data_store, dict): | |
| raise TypeError('data_store must be a dict') | |
| self.val_data = data_store | |
| val_data: dict[str, Any] | |
| def __init__(self, data_store: dict[str, Any]) -> None: | |
| """ | |
| make a new factor and use data_store as a validation tool | |
| :param data_store: the data dictionary holding the base values for the model | |
| """ | |
| if not isinstance(data_store, dict): | |
| raise TypeError('data_store must be a dict') | |
| self.val_data = data_store |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 70-70: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
[warning] 76-76: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/monte_carlo/mc_run.py` around lines 68 - 77, The __init__
method lacks an explicit return annotation; update the constructor signature of
the class containing val_data to include "-> None" (i.e., def __init__(self,
data_store: dict[str, Any]) -> None:) so it satisfies Ruff ANN204 while
preserving the existing TypeError guard and assignment to self.val_data; ensure
no other behavior changes.
b9ec124 to
bffb6b8
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 (3)
temoa/extensions/stochastics/scenario_creator.py (1)
75-77:⚠️ Potential issue | 🟡 Minor
zip(..., strict=True)will raiseValueErrorif lengths don't match.If
idx_tupleis not a tuple (e.g., a single value for single-column indexed tables) or has a different length thanindex_cols, this will raise an exception and abort the scenario. Consider adding defensive handling:🛡️ Proposed defensive handling
for idx_tuple, current_val in list(target_param.items()): # Map index tuple to names based on table manifest - index_map = dict(zip(index_cols, idx_tuple, strict=True)) + key = idx_tuple if isinstance(idx_tuple, tuple) else (idx_tuple,) + if len(key) != len(index_cols): + logger.warning( + 'Index/key length mismatch for table %s: expected %d cols, got %d', + p.table, len(index_cols), len(key), + ) + continue + index_map = dict(zip(index_cols, key, strict=True))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/stochastics/scenario_creator.py` around lines 75 - 77, The code uses dict(zip(index_cols, idx_tuple, strict=True)) which will raise ValueError when idx_tuple isn't the same length as index_cols (e.g., a single value for single-column indexes); update the logic around the loop over target_param.items() to defensively normalize idx_tuple (if not a tuple/list, wrap it into a single-element tuple) and/or explicitly check lengths before calling zip, then build index_map from zip without strict or raise a clearer error; refer to the variables index_cols, idx_tuple, index_map and the loop over target_param.items() when implementing the fix.temoa/extensions/myopic/myopic_sequencer.py (1)
164-176:⚠️ Potential issue | 🟡 MinorMove the missing-config check to the top of
start().
start()callscharacterize_run(),execute_script(),clear_old_results(), andinitialize_myopic_efficiency_table()before theself.config is Noneguard. On the supportedconfig=Noneconstruction path, this still blows up onself.cursor/self.output_coninstead of raising the intendedRuntimeError.🛠️ Minimal fix
def start(self) -> None: + if self.config is None: + raise RuntimeError('Config is not initialized') + # load up the instance queue self.characterize_run() @@ - if self.config is None: - raise RuntimeError('Config is not initialized') - logger.info('Starting Myopic Sequence')Also applies to: 196-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/myopic/myopic_sequencer.py` around lines 164 - 176, Move the missing-config check to the very top of the start() method so that if self.config is None you raise the intended RuntimeError before calling characterize_run(), execute_script(), clear_old_results(), or initialize_myopic_efficiency_table(); locate start() and add the self.config is None guard at the top (same change should be applied to the similar guard usage around the code at the referenced 196-197 area) so no operations touch self.cursor or self.output_con when config is missing.temoa/extensions/modeling_to_generate_alternatives/hull.py (1)
119-130:⚠️ Potential issue | 🟠 MajorRaise on dimension mismatch instead of appending anyway.
After logging the mismatch,
add_point()still stores the bad point. That either leavesall_pointsin an invalid shape or defers the failure to a harder-to-diagnosenp.vstack/ConvexHullerror.🛠️ Fail fast here
def add_point(self, point: np.ndarray) -> None: if len(point) != self.dim: - logger.error( - 'Tried adding a point to hull (dim: %d) with wrong dimensions %d. Point: %s', - self.dim, - len(point), - point, - ) + msg = ( + f'Tried adding a point to hull (dim: {self.dim}) ' + f'with wrong dimensions {len(point)}. Point: {point}' + ) + logger.error(msg) + raise ValueError(msg) if self.all_points is None: self.all_points = np.atleast_2d(point) else: self.all_points = np.vstack((self.all_points, point))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/modeling_to_generate_alternatives/hull.py` around lines 119 - 130, The add_point method currently logs a dimension mismatch but continues to append the bad point; change add_point to validate len(point) != self.dim and immediately raise a ValueError (or TypeError if preferred) with a clear message instead of proceeding, so no modifications to self.all_points occur; update references in add_point (self.dim, self.all_points, np.atleast_2d, np.vstack) to only run after the size check passes and ensure the error message includes the provided point and its length.
♻️ Duplicate comments (15)
temoa/extensions/stochastics/stochastic_sequencer.py (1)
43-47: 🧹 Nitpick | 🔵 TrivialConsider including the original error message in the RuntimeError.
As noted in a previous review, the original
ImportErrormessage varies (e.g., module name vs. submodule issues) and would aid debugging:Suggested improvement
except ImportError as e: logger.exception('mpi-sppy is not installed. Please install it to use stochastic mode.') - raise RuntimeError('mpi-sppy not found') from e + raise RuntimeError(f'mpi-sppy not found: {e}') from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/stochastics/stochastic_sequencer.py` around lines 43 - 47, The RuntimeError raised after catching ImportError in the try/except around "from mpisppy.opt.ef import ExtensiveForm" should include the original ImportError message for better diagnostics; update the raise in that except block to incorporate the caught exception's message (e.g., include str(e) or repr(e) in the RuntimeError text) while keeping the existing logger.exception call and "from e" chaining so the original traceback is preserved.temoa/extensions/stochastics/scenario_creator.py (2)
43-47: 🧹 Nitpick | 🔵 TrivialStandalone type annotation for loop variable is unconventional.
The
item: LoadItemannotation before the for loop (line 44) is valid but uncommon. This style works but could be simplified ifhybrid_loader.manifestis properly typed to returnIterable[LoadItem].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/stochastics/scenario_creator.py` around lines 43 - 47, Remove the standalone loop-variable annotation and instead ensure hybrid_loader.manifest is typed to yield LoadItem so the for loop declares the variable naturally; specifically, delete the separate "item: LoadItem" statement and rely on "for item in hybrid_loader.manifest" with hybrid_loader.manifest typed as Iterable[LoadItem] (or annotate the for-loop as "for item: LoadItem in hybrid_loader.manifest" if you prefer an inline type), leaving the logic that builds table_index_map unchanged (references: table_index_map, item, hybrid_loader.manifest, LoadItem).
57-57: 🧹 Nitpick | 🔵 TrivialThe
castcall uses a string literal for the type argument.While
cast('dict[Any, Any] | None', ...)works (cast accepts strings), the conventional approach withfrom __future__ import annotationsis to use the actual type expression:♻️ Suggested fix
- target_param = cast('dict[Any, Any] | None', data_dict.get(p.table)) + target_param = cast(dict[Any, Any] | None, data_dict.get(p.table))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/stochastics/scenario_creator.py` at line 57, The cast call uses a string literal type; replace cast('dict[Any, Any] | None', data_dict.get(p.table)) with a real type expression: cast(dict[Any, Any] | None, data_dict.get(p.table)) in the scenario_creator code (the target_param assignment) and ensure typing symbols (cast, Any) are imported from typing if not already; no runtime change, just remove the quoted type so static type checkers and from __future__ annotations work correctly.temoa/extensions/myopic/myopic_progress_mapper.py (1)
52-55: 🧹 Nitpick | 🔵 TrivialConsider using
Literalfor thestatusparameter.The
statusparameter accepts only specific string values. UsingLiteralwould catch invalid values at type-check time rather than runtime:♻️ Optional improvement
+from typing import Literal + +StatusType = Literal['load', 'solve', 'report', 'check', 'evolve'] + class MyopicProgressMapper: ... - def report(self, mi: MyopicIndex, status: str) -> None: + def report(self, mi: MyopicIndex, status: StatusType) -> None: if status not in {'load', 'solve', 'report', 'check', 'evolve'}: raise ValueError(f'bad status: {status} received in MyopicProgressMapper')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/myopic/myopic_progress_mapper.py` around lines 52 - 55, The report method on MyopicProgressMapper currently accepts a free-form str for the status parameter and validates at runtime; change the annotation to use typing.Literal with the exact allowed values (Literal['load','solve','report','check','evolve']) for the status parameter in report(self, mi: MyopicIndex, status: ...), add the appropriate import for Literal, and then remove or keep the runtime ValueError as you prefer (runtime check can be removed once callers are fixed and type-checked); also run a quick grep for other call sites of report to update any incorrect static types.temoa/extensions/get_comm_tech.py (2)
34-36: 🧹 Nitpick | 🔵 TrivialSQL query uses string interpolation.
This was flagged in a previous review. While SQL injection is considered low concern per project policy, using parameterized queries would be more consistent with patterns elsewhere in this file:
♻️ Optional fix for consistency
cur.execute( - f"SELECT DISTINCT period FROM output_flow_out WHERE scenario is '{y}'" + "SELECT DISTINCT period FROM output_flow_out WHERE scenario = ?", (y,) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/get_comm_tech.py` around lines 34 - 36, The SQL currently uses f-string interpolation in cur.execute(f"SELECT DISTINCT period FROM output_flow_out WHERE scenario is '{y}'"); change this to a parameterized query using the same DB-API placeholder style used elsewhere in this file (e.g., cur.execute("SELECT DISTINCT period FROM output_flow_out WHERE scenario = %s", (y,)) or cur.execute(..., (y,))) so that cur.execute is called with the query string and a parameters tuple instead of embedding y directly.
207-210: 🧹 Nitpick | 🔵 TrivialException handling improved but still broad.
Changing from bare
except:toexcept Exception:is an improvement. However, a previous review suggested catchingsqlite3.Errorspecifically to avoid masking unexpected errors:♻️ Suggested refinement
try: con = sqlite3.connect(db_file) - except Exception: + except sqlite3.Error: return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/get_comm_tech.py` around lines 207 - 210, The try/except around sqlite3.connect(db_file) is too broad; replace the generic "except Exception" with "except sqlite3.Error as e" so only SQLite errors are caught, and keep the existing failure behavior (e.g., return False) while preserving or logging the error via the caught variable; locate the block using sqlite3.connect, db_file and the con variable in get_comm_tech.py to make this change.temoa/extensions/myopic/myopic_sequencer.py (1)
66-76: 🛠️ Refactor suggestion | 🟠 MajorMake the
config=Noneconstruction path visible in the instance state.
__init__still leavesoutput_con,cursor,table_writer,view_depth, andstep_sizeunset whenconfigisNone, andtests/test_myopic_sequencer.py:59-75still exercises that path. Keeping them as always-present members makes the typed surface lie and leaves future callers open toAttributeError.♻️ One workable pattern
- output_con: Connection - cursor: Cursor - table_writer: TableWriter - view_depth: int - step_size: int + output_con: Connection | None + cursor: Cursor | None + table_writer: TableWriter | None + view_depth: int | None + step_size: int | None @@ def __init__(self, config: TemoaConfig | None) -> None: @@ self.optimization_periods = None self.instance_queue = deque() # a LIFO queue self.progress_mapper = None self.config = config + self.output_con = None + self.cursor = None + self.table_writer = None + self.view_depth = None + self.step_size = None # allow a "shunt" (None) here so we can test parts of this by passing a None config if self.config:Also applies to: 78-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/myopic/myopic_sequencer.py` around lines 66 - 76, The constructor __init__ leaves attributes output_con, cursor, table_writer, view_depth and step_size unset when config is None, so update __init__ to explicitly initialize those members in the config=None path (e.g., set output_con, cursor, table_writer = None and view_depth, step_size = None) and change their type annotations to Optional[...] (or union with None) so the instance state matches the typed surface; make these changes in myopic_sequencer.py around the __init__ handling of config to ensure tests in tests/test_myopic_sequencer.py exercise the None path without causing AttributeError.temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py (1)
376-377:⚠️ Potential issue | 🟡 MinorGuard
__del__()against partial construction.
__init__can raise beforeself.conis assigned, so unconditionalself.con.close()can emit a secondaryAttributeErrorduring garbage collection and obscure the real failure.🛠️ Minimal fix
def __del__(self) -> None: - self.con.close() + con = getattr(self, 'con', None) + if con is not None: + con.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py` around lines 376 - 377, The __del__ method currently calls self.con.close() unconditionally which can raise AttributeError if __init__ failed before assigning self.con; update the __del__ in this class (mga_sequencer.py) to guard against partial construction by checking for the presence and truthiness of self.con (e.g. if hasattr(self, "con") and self.con is not None) before calling close(), and optionally wrap the close() call in a try/except to swallow/ log any exceptions during finalization so secondary errors do not mask the original construction failure.temoa/extensions/method_of_morris/morris.py (2)
51-56:⚠️ Potential issue | 🔴 CriticalFix the
output_emissiontable name.The query still targets
output_emissionn, which does not match the schema/table-writer name. This will fail everyevaluate()call before the Morris analysis completes.🐛 Minimal fix
- cur.execute("SELECT emis_comm, SUM(emission) FROM output_emissionn WHERE emis_comm='co2'") + cur.execute("SELECT emis_comm, SUM(emission) FROM output_emission WHERE emis_comm='co2'")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris.py` around lines 51 - 56, The SQL query uses the misspelled table name "output_emissionn" which will cause evaluate() to fail; update the SQL in the cur.execute call that currently reads "SELECT emis_comm, SUM(emission) FROM output_emissionn WHERE emis_comm='co2'" to use the correct table name "output_emission" so the query matches the writer/schema (ensure you edit the cur.execute string in morris.py where output_query is assigned).
24-25:⚠️ Potential issue | 🟠 MajorDon't use
db_fileafter theas_file()context exits.
evaluate()runs after thewith resources.as_file(...)block has completed, but it still opens the globaldb_file. That path can be a cleaned-up temporary extraction, so Morris evaluations can fail as soon as the resource is no longer materialized. Keep the evaluation phase inside the context or materialize a stable DB path and pass it intoevaluate().In Python 3.12, how long is the path yielded by `importlib.resources.as_file()` guaranteed to remain valid? Does it remain valid after the `with` block exits?Also applies to: 42-50, 72-74, 155-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris.py` around lines 24 - 25, The evaluate() function currently opens/uses the global db_file path after the importlib.resources.as_file(...) context has exited, which can point to a removed temporary extraction; fix by ensuring the database path is stable when evaluate() runs: either move the call(s) to evaluate(...) so they occur inside the with resources.as_file(...) context, or change evaluate(param_names, param_values, data, k) to accept an explicit, materialized db_path (e.g., pass a temporary permanent copy/path created inside the with block) and use that stable path instead of relying on a global db_file; update all call sites (where evaluate is invoked) accordingly and remove usage of the global db_file outside the as_file() context.temoa/extensions/single_vector_mga/sv_mga_sequencer.py (1)
125-133:⚠️ Potential issue | 🟠 MajorValidate label settings before building the alternate objective.
These values are consumed as iterables of labels. If the TOML supplies a scalar string,
construct_obj()will iterate characters and build a nonsense objective. Normalize them tolist[str]here (handling single strings explicitly) instead of forwarding raw config values.🛠️ Example normalization
+ def _normalize_labels(name: str, value: object) -> list[str]: + if value is None: + return [] + if isinstance(value, str): + return [value] + if isinstance(value, Iterable): + return [str(x) for x in value] + raise TypeError(f'{name} must be a string or an iterable of strings') + svmga_inputs = self.config.svmga_inputs or {} - emission_labels = svmga_inputs.get('emission_labels', []) - capacity_labels = svmga_inputs.get('capacity_labels', []) - activity_labels = svmga_inputs.get('activity_labels', []) + emission_labels = _normalize_labels( + 'emission_labels', svmga_inputs.get('emission_labels') + ) + capacity_labels = _normalize_labels( + 'capacity_labels', svmga_inputs.get('capacity_labels') + ) + activity_labels = _normalize_labels( + 'activity_labels', svmga_inputs.get('activity_labels') + ) new_obj = SvMgaSequencer.construct_obj( instance, - emission_labels, # type: ignore[arg-type] - capacity_labels, # type: ignore[arg-type] - activity_labels, # type: ignore[arg-type] + emission_labels, + capacity_labels, + activity_labels, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/single_vector_mga/sv_mga_sequencer.py` around lines 125 - 133, The config may supply scalar strings which would be iterated character-by-character by SvMgaSequencer.construct_obj; before calling construct_obj normalize svmga_inputs values (emission_labels, capacity_labels, activity_labels) into list[str] values: if the value is None -> [], if it's a str -> [value], if it's already an iterable/list leave as-is (or cast to list of str), and otherwise coerce/raise; replace the direct assignments to emission_labels/capacity_labels/activity_labels with this normalization so construct_obj always receives proper list[str] arguments.temoa/extensions/monte_carlo/mc_run.py (1)
224-228:⚠️ Potential issue | 🟠 MajorFail fast when
run_settingsis missing.Line 228 still indexes the optional
monte_carlo_inputsdict without validating it first. A missing Monte Carlo block or missingrun_settingskey becomes aTypeError/KeyErrorduring factory construction instead of a clear configuration error.Suggested fix
- def __init__(self, config: TemoaConfig, data_store: dict[str, Any]): + def __init__(self, config: TemoaConfig, data_store: dict[str, Any]) -> None: self.config = config self.data_store = data_store self.tweak_factory = TweakFactory(data_store) - self.settings_file = Path(self.config.monte_carlo_inputs['run_settings']) # type: ignore + monte_carlo_inputs = self.config.monte_carlo_inputs + if monte_carlo_inputs is None or 'run_settings' not in monte_carlo_inputs: + raise ValueError('Monte Carlo configuration must define run_settings') + self.settings_file = Path(str(monte_carlo_inputs['run_settings']))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/mc_run.py` around lines 224 - 228, In TweakRun.__init__, validate that the TemoaConfig's monte_carlo_inputs exists and contains the 'run_settings' key before indexing it: check that self.config.monte_carlo_inputs is a dict-like object and that 'run_settings' in self.config.monte_carlo_inputs, and if not raise a clear ValueError (or ConfigError) explaining the missing Monte Carlo configuration and which key is absent; then set self.settings_file = Path(self.config.monte_carlo_inputs['run_settings']) and proceed to construct TweakFactory(data_store). Ensure you reference the __init__ method, TemoaConfig.monte_carlo_inputs, settings_file, and TweakFactory in your change.temoa/extensions/monte_carlo/mc_worker.py (1)
46-61: 🛠️ Refactor suggestion | 🟠 MajorMake
solver_nameandsolver_optionsexplicit constructor parameters.Lines 60-61 still hide two required inputs behind
**kwargs, which keeps theKeyErrorpath if a caller omits either key and prevents the type checker from enforcing the worker API.Suggested fix
def __init__( self, dp_queue: Queue[Any], results_queue: Queue[DataBrick | str], log_root_name: str, log_queue: Queue[logging.LogRecord], log_level: int = logging.INFO, + solver_name: str = 'appsi_highs', + solver_options: dict[str, Any] | None = None, solver_log_path: Path | None = None, - **kwargs: Any, - ): + ) -> None: self.worker_number = MCWorker.worker_idx MCWorker.worker_idx += 1 self.dp_queue = dp_queue self.results_queue = results_queue - self.solver_name = kwargs['solver_name'] - self.solver_options = kwargs['solver_options'] + self.solver_name = solver_name + self.solver_options = solver_options or {}Run this to inspect the current constructor and its call sites before tightening the signature; the expected result is that each
MCWorker(...)instantiation already passessolver_nameandsolver_optionsexplicitly.#!/bin/bash sed -n '46,66p' temoa/extensions/monte_carlo/mc_worker.py printf '\n--- MCWorker call sites ---\n' rg -nC4 --type=py '\bMCWorker\s*\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/mc_worker.py` around lines 46 - 61, The constructor for MCWorker hides required args in **kwargs causing KeyError and weakening typing; update MCWorker.__init__ to accept solver_name: str and solver_options: dict (or the precise types used) as explicit parameters (in addition to dp_queue, results_queue, log_root_name, log_queue, log_level, solver_log_path), remove the self.solver_name = kwargs['solver_name'] and self.solver_options = kwargs['solver_options'] accesses, and assign the passed parameters to self.solver_name / self.solver_options; then update every MCWorker(...) call site to pass solver_name and solver_options explicitly and run a project-wide search for "MCWorker(" to verify all call sites are updated.temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py (1)
147-148:⚠️ Potential issue | 🟠 MajorReturn variable names for the requested technology.
Line 148 ignores
techand returns category labels instead. That violatesVectorManager.group_variable_names()’s contract and gives callers unrelated names whenever they ask for a specific technology.Suggested fix
def group_variable_names(self, tech: str) -> list[str]: - return [str(k) for k in self.category_mapping.keys()] + return list(self.variable_index_mapping.get(tech, {}).keys())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py` around lines 147 - 148, group_variable_names currently ignores its tech parameter and returns all category_mapping keys (category labels) which violates VectorManager.group_variable_names contract; update group_variable_names to look up the entry for the given tech in category_mapping (use self.category_mapping[tech] or self.category_mapping.get(tech)) and return the variable names for that technology (convert to str as needed), and handle missing techs by returning an empty list or raising an appropriate error so callers receive only the variables for the requested tech.temoa/extensions/single_vector_mga/output_summary.py (1)
31-52:⚠️ Potential issue | 🟡 MinorDon’t collapse valid zero values and missing data into the same case.
all((orig, option))treats a real0.0as false, so a legitimate drop-to-zero becomesNoneinstead of-100%. The helpers also still return0.0for both “no row” and “SUM(...) == 0”, so the summary can’t distinguish missing data from a valid zero.Suggested fix
- row_delta = float((option - orig) / orig * 100) if all((orig, option)) else None + row_delta = ( + (option - orig) / orig * 100 + if orig is not None and option is not None and orig != 0 + else None + ) @@ - row_delta = (option - orig) / orig * 100 if all((orig, option)) else None + row_delta = ( + (option - orig) / orig * 100 + if orig is not None and option is not None and orig != 0 + else None + ) @@ -def poll_emission(conn: sqlite3.Connection, scenario: str, label: str) -> float: +def poll_emission(conn: sqlite3.Connection, scenario: str, label: str) -> float | None: @@ - return float(row[0] if row and row[0] is not None else 0.0) + return float(row[0]) if row and row[0] is not None else NoneApply the same
None-returning pattern topoll_activity()andpoll_capacity().Also applies to: 64-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/single_vector_mga/output_summary.py` around lines 31 - 52, The delta logic is incorrectly treating 0.0 as missing because it uses truthiness (all((orig, option))); update the code so poll_activity and poll_capacity mirror poll_emission’s behavior by returning None for missing rows (distinct from 0.0), and change the row_delta guards in output_summary.py to test for None (e.g., "orig is not None and option is not None") before computing (option - orig)/orig*100 so that valid zeros yield -100% and missing data stays None; locate changes around the poll_activity, poll_capacity function implementations and the row_delta computations that currently use all((orig, option)).
🤖 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/extensions/method_of_morris/morris_evaluate.py`:
- Around line 91-106: The code reads scalar aggregates into y_of and
y_cumulative_co2 from output_query and then casts to float, but it doesn't guard
against the DB returning a row with a NULL value (None), which causes a
TypeError; update the logic around where y_of and y_cumulative_co2 are assigned
(the output_query handling after the cur.execute calls that use scenario_name)
to treat a fetched value of None as 0.0 (or a safe default) before constructing
morris_objectives, i.e., if output_query is empty keep the existing 0.0 branch,
and if output_query has one row but output_query[0][0] is None set
y_of/y_cumulative_co2 = 0.0 instead of passing None into float().
In `@temoa/extensions/method_of_morris/morris_sequencer.py`:
- Around line 120-121: The current assignment in morris_sequencer.py uses
truthiness so a seed value of 0 becomes None; change the check around
morris_inputs.get('seed') so you only treat the seed as missing when it is None
(e.g., use "if seed is not None" or test for key presence) and then set
self.seed = int(cast('Any', seed)) when present; update the logic where seed is
retrieved and assigned (the seed variable and self.seed assignment) to preserve
0 as a valid deterministic seed.
---
Outside diff comments:
In `@temoa/extensions/modeling_to_generate_alternatives/hull.py`:
- Around line 119-130: The add_point method currently logs a dimension mismatch
but continues to append the bad point; change add_point to validate len(point)
!= self.dim and immediately raise a ValueError (or TypeError if preferred) with
a clear message instead of proceeding, so no modifications to self.all_points
occur; update references in add_point (self.dim, self.all_points, np.atleast_2d,
np.vstack) to only run after the size check passes and ensure the error message
includes the provided point and its length.
In `@temoa/extensions/myopic/myopic_sequencer.py`:
- Around line 164-176: Move the missing-config check to the very top of the
start() method so that if self.config is None you raise the intended
RuntimeError before calling characterize_run(), execute_script(),
clear_old_results(), or initialize_myopic_efficiency_table(); locate start() and
add the self.config is None guard at the top (same change should be applied to
the similar guard usage around the code at the referenced 196-197 area) so no
operations touch self.cursor or self.output_con when config is missing.
In `@temoa/extensions/stochastics/scenario_creator.py`:
- Around line 75-77: The code uses dict(zip(index_cols, idx_tuple, strict=True))
which will raise ValueError when idx_tuple isn't the same length as index_cols
(e.g., a single value for single-column indexes); update the logic around the
loop over target_param.items() to defensively normalize idx_tuple (if not a
tuple/list, wrap it into a single-element tuple) and/or explicitly check lengths
before calling zip, then build index_map from zip without strict or raise a
clearer error; refer to the variables index_cols, idx_tuple, index_map and the
loop over target_param.items() when implementing the fix.
---
Duplicate comments:
In `@temoa/extensions/get_comm_tech.py`:
- Around line 34-36: The SQL currently uses f-string interpolation in
cur.execute(f"SELECT DISTINCT period FROM output_flow_out WHERE scenario is
'{y}'"); change this to a parameterized query using the same DB-API placeholder
style used elsewhere in this file (e.g., cur.execute("SELECT DISTINCT period
FROM output_flow_out WHERE scenario = %s", (y,)) or cur.execute(..., (y,))) so
that cur.execute is called with the query string and a parameters tuple instead
of embedding y directly.
- Around line 207-210: The try/except around sqlite3.connect(db_file) is too
broad; replace the generic "except Exception" with "except sqlite3.Error as e"
so only SQLite errors are caught, and keep the existing failure behavior (e.g.,
return False) while preserving or logging the error via the caught variable;
locate the block using sqlite3.connect, db_file and the con variable in
get_comm_tech.py to make this change.
In `@temoa/extensions/method_of_morris/morris.py`:
- Around line 51-56: The SQL query uses the misspelled table name
"output_emissionn" which will cause evaluate() to fail; update the SQL in the
cur.execute call that currently reads "SELECT emis_comm, SUM(emission) FROM
output_emissionn WHERE emis_comm='co2'" to use the correct table name
"output_emission" so the query matches the writer/schema (ensure you edit the
cur.execute string in morris.py where output_query is assigned).
- Around line 24-25: The evaluate() function currently opens/uses the global
db_file path after the importlib.resources.as_file(...) context has exited,
which can point to a removed temporary extraction; fix by ensuring the database
path is stable when evaluate() runs: either move the call(s) to evaluate(...) so
they occur inside the with resources.as_file(...) context, or change
evaluate(param_names, param_values, data, k) to accept an explicit, materialized
db_path (e.g., pass a temporary permanent copy/path created inside the with
block) and use that stable path instead of relying on a global db_file; update
all call sites (where evaluate is invoked) accordingly and remove usage of the
global db_file outside the as_file() context.
In `@temoa/extensions/modeling_to_generate_alternatives/mga_sequencer.py`:
- Around line 376-377: The __del__ method currently calls self.con.close()
unconditionally which can raise AttributeError if __init__ failed before
assigning self.con; update the __del__ in this class (mga_sequencer.py) to guard
against partial construction by checking for the presence and truthiness of
self.con (e.g. if hasattr(self, "con") and self.con is not None) before calling
close(), and optionally wrap the close() call in a try/except to swallow/ log
any exceptions during finalization so secondary errors do not mask the original
construction failure.
In
`@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py`:
- Around line 147-148: group_variable_names currently ignores its tech parameter
and returns all category_mapping keys (category labels) which violates
VectorManager.group_variable_names contract; update group_variable_names to look
up the entry for the given tech in category_mapping (use
self.category_mapping[tech] or self.category_mapping.get(tech)) and return the
variable names for that technology (convert to str as needed), and handle
missing techs by returning an empty list or raising an appropriate error so
callers receive only the variables for the requested tech.
In `@temoa/extensions/monte_carlo/mc_run.py`:
- Around line 224-228: In TweakRun.__init__, validate that the TemoaConfig's
monte_carlo_inputs exists and contains the 'run_settings' key before indexing
it: check that self.config.monte_carlo_inputs is a dict-like object and that
'run_settings' in self.config.monte_carlo_inputs, and if not raise a clear
ValueError (or ConfigError) explaining the missing Monte Carlo configuration and
which key is absent; then set self.settings_file =
Path(self.config.monte_carlo_inputs['run_settings']) and proceed to construct
TweakFactory(data_store). Ensure you reference the __init__ method,
TemoaConfig.monte_carlo_inputs, settings_file, and TweakFactory in your change.
In `@temoa/extensions/monte_carlo/mc_worker.py`:
- Around line 46-61: The constructor for MCWorker hides required args in
**kwargs causing KeyError and weakening typing; update MCWorker.__init__ to
accept solver_name: str and solver_options: dict (or the precise types used) as
explicit parameters (in addition to dp_queue, results_queue, log_root_name,
log_queue, log_level, solver_log_path), remove the self.solver_name =
kwargs['solver_name'] and self.solver_options = kwargs['solver_options']
accesses, and assign the passed parameters to self.solver_name /
self.solver_options; then update every MCWorker(...) call site to pass
solver_name and solver_options explicitly and run a project-wide search for
"MCWorker(" to verify all call sites are updated.
In `@temoa/extensions/myopic/myopic_progress_mapper.py`:
- Around line 52-55: The report method on MyopicProgressMapper currently accepts
a free-form str for the status parameter and validates at runtime; change the
annotation to use typing.Literal with the exact allowed values
(Literal['load','solve','report','check','evolve']) for the status parameter in
report(self, mi: MyopicIndex, status: ...), add the appropriate import for
Literal, and then remove or keep the runtime ValueError as you prefer (runtime
check can be removed once callers are fixed and type-checked); also run a quick
grep for other call sites of report to update any incorrect static types.
In `@temoa/extensions/myopic/myopic_sequencer.py`:
- Around line 66-76: The constructor __init__ leaves attributes output_con,
cursor, table_writer, view_depth and step_size unset when config is None, so
update __init__ to explicitly initialize those members in the config=None path
(e.g., set output_con, cursor, table_writer = None and view_depth, step_size =
None) and change their type annotations to Optional[...] (or union with None) so
the instance state matches the typed surface; make these changes in
myopic_sequencer.py around the __init__ handling of config to ensure tests in
tests/test_myopic_sequencer.py exercise the None path without causing
AttributeError.
In `@temoa/extensions/single_vector_mga/output_summary.py`:
- Around line 31-52: The delta logic is incorrectly treating 0.0 as missing
because it uses truthiness (all((orig, option))); update the code so
poll_activity and poll_capacity mirror poll_emission’s behavior by returning
None for missing rows (distinct from 0.0), and change the row_delta guards in
output_summary.py to test for None (e.g., "orig is not None and option is not
None") before computing (option - orig)/orig*100 so that valid zeros yield -100%
and missing data stays None; locate changes around the poll_activity,
poll_capacity function implementations and the row_delta computations that
currently use all((orig, option)).
In `@temoa/extensions/single_vector_mga/sv_mga_sequencer.py`:
- Around line 125-133: The config may supply scalar strings which would be
iterated character-by-character by SvMgaSequencer.construct_obj; before calling
construct_obj normalize svmga_inputs values (emission_labels, capacity_labels,
activity_labels) into list[str] values: if the value is None -> [], if it's a
str -> [value], if it's already an iterable/list leave as-is (or cast to list of
str), and otherwise coerce/raise; replace the direct assignments to
emission_labels/capacity_labels/activity_labels with this normalization so
construct_obj always receives proper list[str] arguments.
In `@temoa/extensions/stochastics/scenario_creator.py`:
- Around line 43-47: Remove the standalone loop-variable annotation and instead
ensure hybrid_loader.manifest is typed to yield LoadItem so the for loop
declares the variable naturally; specifically, delete the separate "item:
LoadItem" statement and rely on "for item in hybrid_loader.manifest" with
hybrid_loader.manifest typed as Iterable[LoadItem] (or annotate the for-loop as
"for item: LoadItem in hybrid_loader.manifest" if you prefer an inline type),
leaving the logic that builds table_index_map unchanged (references:
table_index_map, item, hybrid_loader.manifest, LoadItem).
- Line 57: The cast call uses a string literal type; replace cast('dict[Any,
Any] | None', data_dict.get(p.table)) with a real type expression:
cast(dict[Any, Any] | None, data_dict.get(p.table)) in the scenario_creator code
(the target_param assignment) and ensure typing symbols (cast, Any) are imported
from typing if not already; no runtime change, just remove the quoted type so
static type checkers and from __future__ annotations work correctly.
In `@temoa/extensions/stochastics/stochastic_sequencer.py`:
- Around line 43-47: The RuntimeError raised after catching ImportError in the
try/except around "from mpisppy.opt.ef import ExtensiveForm" should include the
original ImportError message for better diagnostics; update the raise in that
except block to incorporate the caught exception's message (e.g., include str(e)
or repr(e) in the RuntimeError text) while keeping the existing logger.exception
call and "from e" chaining so the original traceback is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a41d71f-31d1-462c-aae4-af30c2b00fb5
📒 Files selected for processing (24)
pyproject.tomltemoa/extensions/get_comm_tech.pytemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/method_of_morris/morris_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/hull.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/example_builds/scenario_analyzer.pytemoa/extensions/monte_carlo/example_builds/scenario_maker.pytemoa/extensions/monte_carlo/mc_run.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/myopic/myopic_index.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.py
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
temoa/extensions/stochastics/scenario_creator.py (1)
37-50:⚠️ Potential issue | 🟡 MinorDon't relabel manifest/data-load failures as connection failures.
The
tryblock now coverscreate_data_dict()and manifest walking too, so an error there is rethrown as "Failed to connect to database". Either narrow thetrytosqlite3.connect(...)or broaden the message to "load base data" so diagnostics stay accurate.Minimal fix
- except Exception as e: - logger.exception('Failed to connect to database %s', temoa_config.input_database) - raise RuntimeError(f'Failed to connect to database {temoa_config.input_database}') from e + except Exception as e: + logger.exception('Failed to load base data from database %s', temoa_config.input_database) + raise RuntimeError( + f'Failed to load base data from database {temoa_config.input_database}' + ) from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/stochastics/scenario_creator.py` around lines 37 - 50, The try/except currently wraps sqlite3.connect plus HybridLoader.create_data_dict() and manifest iteration, causing manifest or data-load errors to be reported as "Failed to connect to database"; restrict the try to only the connection call or change the error message to reflect loading base data: e.g., move the with sqlite3.connect(temoa_config.input_database) as con: into its own try/except (catch connection errors only) and then run HybridLoader(db_connection=con) and hybrid_loader.create_data_dict(...) outside that connection-only try, or alternatively update the except message to "Failed to load base data from {temoa_config.input_database}" so failures in HybridLoader.create_data_dict and iterating hybrid_loader.manifest are not misreported; refer to HybridLoader, create_data_dict, and hybrid_loader.manifest when making the change.temoa/extensions/monte_carlo/mc_run.py (2)
175-185: 🛠️ Refactor suggestion | 🟠 MajorAdd
-> Nonereturn type toMCRun.__init__.Proposed fix
def __init__( self, scenario_name: str, run_index: int, data_store: dict[str, Any], included_tweaks: dict[Tweak, list[ChangeRecord]], - ): + ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/mc_run.py` around lines 175 - 185, The __init__ method of MCRun is missing an explicit return type annotation; update the MCRun.__init__ signature to include -> None (i.e., def __init__(self, scenario_name: str, run_index: int, data_store: dict[str, Any], included_tweaks: dict[Tweak, list[ChangeRecord]]) -> None:) to make the constructor's return type explicit and satisfy type checkers; keep the body unchanged.
43-54: 🛠️ Refactor suggestion | 🟠 MajorAdd
-> Nonereturn type toTweak.__init__.Proposed fix
- def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float): + def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/mc_run.py` around lines 43 - 54, The __init__ method of the Tweak class is missing an explicit return annotation; update the Tweak.__init__ signature to include "-> None" (i.e., def __init__(self, param_name: str, indices: tuple[Any, ...], adjustment: str, value: float) -> None:) so the constructor has the proper return type annotation and matches typing expectations.temoa/extensions/monte_carlo/mc_sequencer.py (1)
153-156: 🧹 Nitpick | 🔵 TrivialRemove unused
kwargsdict.The
kwargsdictionary is constructed but never used—MCWorkeris instantiated with explicitsolver_nameandsolver_optionsparameters at lines 167-168.Proposed fix
- kwargs = { - 'solver_name': self.config.solver_name, - 'solver_options': self.worker_solver_options, - } # construct path for the solver logs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/mc_sequencer.py` around lines 153 - 156, Remove the unused kwargs dict: the local variable kwargs created in mc_sequencer.py (containing 'solver_name' and 'solver_options') is never passed anywhere and is redundant because MCWorker is instantiated with explicit solver_name and solver_options; delete the kwargs construction to avoid dead code and ensure only the direct MCWorker(...) call (which uses self.config.solver_name and self.worker_solver_options) remains.
♻️ Duplicate comments (9)
temoa/extensions/single_vector_mga/sv_mga_sequencer.py (1)
52-53: 🛠️ Refactor suggestion | 🟠 MajorNormalize
svmga_inputsonce instead of silencing types at each use.
type: ignore[arg-type]oncost_epsilonand thecast(...)calls on the label lists only quiet mypy; they still pass raw config values straight through at runtime. Parse and validate these fields once in__init__, store typed attributes, and pass those through here.Also applies to: 125-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/single_vector_mga/sv_mga_sequencer.py` around lines 52 - 53, Normalize and validate svmga_inputs in the class __init__: parse svmga_inputs = config.svmga_inputs or {}, convert and store typed attributes (e.g., self.svmga_inputs as dict, self.cost_epsilon: float = float(svmga_inputs.get('cost_epsilon', 0.05))) and validate/normalize label lists (e.g., self.start_labels, self.end_labels, self.exact_match_labels) to ensure they are List[str] (coerce/split/raise on bad types), then remove inline cast(...) and type: ignore[arg-type] usages in methods like sv_mga_sequencer.__init__ and the block around lines 125-133 so those methods consume the already-validated self attributes rather than raw config values.temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py (2)
17-19:⚠️ Potential issue | 🟠 MajorGuard the empty-result case before deriving histogram bins.
If the scenario prefix matches no rows,
len(obj_values_tuple)is0,binsbecomes0, andplt.hist(..., bins=0)raises instead of failing gracefully.Suggested fix
obj_values_tuple = tuple(t[0] for t in obj_values) -plt.hist(obj_values_tuple, bins=int(sqrt(len(obj_values_tuple)))) +if not obj_values_tuple: + raise SystemExit(f'No objective values found for scenario prefix: {scenario_name!r}') + +plt.hist(obj_values_tuple, bins=max(1, int(sqrt(len(obj_values_tuple))))) plt.show()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py` around lines 17 - 19, The histogram code assumes obj_values_tuple is non-empty, so compute obj_values_tuple (from obj_values) and guard the empty-result case before calling plt.hist: if obj_values_tuple is empty, skip plotting or handle it (e.g., log/return or set a minimum bins value) instead of calling plt.hist with bins=int(sqrt(len(obj_values_tuple))); update the code around obj_values_tuple and the plt.hist call to check len(obj_values_tuple) == 0 and handle that branch safely.
4-12:⚠️ Potential issue | 🟠 MajorThis
importlib.resourceslookup targets data outside the packaged wheel.
resources.files('data_files.example_dbs')only works for installed package resources, but the wheel target here shipstemoa/**only. Unless the sample DB is moved undertemoaand declared as package data, this example will not be able to resolve its database after installation.#!/bin/bash # Verify that the referenced resource package/data are actually shippable. echo "=== wheel target ===" sed -n '76,81p' pyproject.toml echo echo "=== package markers under data_files ===" fd "__init__.py" . | rg '^data_files/' || true echo echo "=== example DB assets ===" fd "utopia.sqlite" . || true fd "utopia.sql" . || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py` around lines 4 - 12, The importlib.resources lookup targets "data_files.example_dbs" which is not part of the installed "temoa" package so db_resource cannot be resolved after installation; update the lookup and packaging so the sample DB is shipped with the temoa package: either move the DB into the temoa package and declare it as package data in pyproject.toml, then change the resolver line (db_resource = resources.files('temoa.data_files.example_dbs') / 'utopia.sqlite') to point at the temoa package, or implement a fallback that opens the DB from a filesystem path relative to this module (using Path(__file__).parent) when resources.files('data_files.example_dbs') fails; ensure the symbol db_resource (and the with resources.as_file(...) as db_path, Connection(...) usage) uses the corrected resource path so Connection(str(db_path)) will work post-install.temoa/extensions/modeling_to_generate_alternatives/hull.py (1)
28-33: 🛠️ Refactor suggestion | 🟠 MajorStop accepting ignored
**kwargsinHull.__init__.Unexpected keyword arguments are silently discarded today, so typoed options look accepted but have no effect. Either remove
**kwargsor replace it with explicit keyword-only parameters.#!/bin/bash # Check whether any current Hull call sites rely on keyword arguments. python - <<'PY' import ast import pathlib for path in pathlib.Path('.').rglob('*.py'): try: tree = ast.parse(path.read_text()) except Exception: continue for node in ast.walk(tree): if isinstance(node, ast.Call): func = node.func name = None if isinstance(func, ast.Name): name = func.id elif isinstance(func, ast.Attribute): name = func.attr if name == 'Hull' and node.keywords: print(f"{path}:{node.lineno}: {[kw.arg for kw in node.keywords]}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/modeling_to_generate_alternatives/hull.py` around lines 28 - 33, The __init__ of Hull currently accepts and ignores **kwargs which hides typos; remove the **kwargs from Hull.__init__ signature (def __init__(self, points: np.ndarray) -> None:) or replace it with explicit keyword-only parameters if there are legitimate optional settings (e.g., def __init__(self, points: np.ndarray, *, tol: float = 1e-9) -> None:); update any internal references to kwargs in the Hull class (none expected) and run the provided AST-check script to find call sites that pass keyword args so you can either update those call sites to use the new explicit parameters or remove the keywords. Ensure the function docstring and any tests reflect the new signature.temoa/extensions/method_of_morris/morris_evaluate.py (2)
82-91:⚠️ Potential issue | 🟠 MajorHandle the empty objective query before indexing
[0].The new fallback only covers
NULLinside a returned row. Ifoutput_objectivehas no row forscenario_name,output_query[0][0]still raisesIndexErrorand hides the real failure.🛡️ Missing-row guard
cur.execute( 'SELECT total_system_cost FROM output_objective where scenario = ?', (scenario_name,) ) output_query = cur.fetchall() + if len(output_query) == 0: + raise RuntimeError(f'No objective output found for scenario {scenario_name}') if len(output_query) > 1: raise RuntimeError( 'Multiple outputs found in Objective table matching scenario name. Coding error.' ) - else: - y_of = output_query[0][0] if output_query[0][0] is not None else 0.0 + y_of = output_query[0][0] if output_query[0][0] is not None else 0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris_evaluate.py` around lines 82 - 91, The code reads total_system_cost into output_query then blindly indexes output_query[0][0]; add a guard that checks if output_query is empty before indexing (e.g., in the block where y_of is assigned) and handle the missing-row case explicitly—either raise a clear RuntimeError indicating no Objective row for the given scenario_name or set y_of to a sensible default (0.0) depending on expected behavior; update the logic around output_query and y_of in morris_evaluate.py to avoid IndexError and include scenario_name in the error/log for diagnostics.
64-66:⚠️ Potential issue | 🟡 MinorFormat the debug message before appending it.
This still appends the raw
%d/%s/%ftemplate, so the worker debug log never shows which parameter changed.🪵 Logging fix
- setting_entry = 'run # %d: Setting param %s[%s] to value: %f' - log_entry.append(setting_entry) + log_entry.append( + f'run # {i + 1}: Setting param {param_name}[{set_idx_tuple}] to value: ' + f'{float(mm_sample[j]):.6g}' + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris_evaluate.py` around lines 64 - 66, The code constructs a format template in variable setting_entry but appends the raw template to log_entry; change it to format the message before appending so the actual values are logged — e.g., replace log_entry.append(setting_entry) with formatting using the current variables (for example: setting_entry % (j, param_name, set_idx_tuple, mm_sample[j]) or an f-string) right after data[param_name][set_idx_tuple] = mm_sample[j]; ensure the formatted string (not the template) is appended to log_entry so the worker debug log shows which parameter changed.temoa/extensions/get_comm_tech.py (1)
12-19:⚠️ Potential issue | 🟡 MinorUse
ValueErrorfor invalid input here.These branches are rejecting caller-supplied file arguments, so
ValueErroris the precise signal. KeepingExceptionhere leaves the earlier review item unresolved and makes upstream error handling coarser than it needs to be.Also applies to: 47-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/get_comm_tech.py` around lines 12 - 19, Replace the broad Exception raises with ValueError for invalid caller-supplied file arguments: in get_tperiods() change the two raise Exception(...) calls (the "file type not recognized" branch and the "Please specify a database" branch) to raise ValueError(...) so callers can handle input validation errors precisely; also make the analogous change in the similar validation block around lines 47-54 in this file (replace those raise Exception(...) occurrences with ValueError(...)).temoa/extensions/method_of_morris/morris_sequencer.py (1)
120-121:⚠️ Potential issue | 🟠 Major
seed=0is still lost during Morris analysis.The parsing change now preserves zero, but the SALib calls at Lines 223 and 234 still use
if self.seed else None, which converts0back toNoneand makes the resampling step nondeterministic again.🎯 Follow-through fix
- seed=self.seed + 1 if self.seed else None, + seed=self.seed + 1 if self.seed is not None else None, @@ - seed=self.seed + 2 if self.seed else None, + seed=self.seed + 2 if self.seed is not None else None,Also applies to: 204-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/method_of_morris/morris_sequencer.py` around lines 120 - 121, The seed parsing now preserves zero but downstream SALib calls still use the truthy check "if self.seed else None", which converts 0 back to None; update every place in morris_sequencer.py that passes the seed (currently written as "if self.seed else None") to instead use an explicit None-check (e.g., "self.seed if self.seed is not None else None") so that seed=0 is passed through to SALib; apply the same fix for all occurrences referenced in the review (the two calls around where SALib sampling/resampling is invoked and the earlier related check).temoa/extensions/modeling_to_generate_alternatives/vector_manager.py (1)
73-76: 🧹 Nitpick | 🔵 TrivialAbstract method
finalize_trackershould raiseNotImplementedErrorinstead ofpass.Other abstract methods in this class raise
NotImplementedError(). For consistency and to catch unimplemented subclasses, replacepasswithraise NotImplementedError().Proposed fix
`@abstractmethod` def finalize_tracker(self) -> None: """Finalize any tracker employed by the manager""" - pass + raise NotImplementedError()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/modeling_to_generate_alternatives/vector_manager.py` around lines 73 - 76, The abstract method finalize_tracker in the VectorManager class currently ends with a bare "pass"; replace that with "raise NotImplementedError()" so that subclasses that don't implement finalize_tracker fail loudly and consistently with the other abstract methods in this class; locate the finalize_tracker method definition and change its body to raise NotImplementedError() with no additional logic.
🤖 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/extensions/get_comm_tech.py`:
- Around line 263-264: get_info currently returns typing.Any; constrain it to
the actual shapes returned (period lists or string-keyed mappings) by replacing
Any with a precise union or alias (e.g., define PeriodList = list[str] and
PeriodMap = dict[str, PeriodList] and use -> PeriodList | PeriodMap or a single
alias like InfoResult). Update the get_info signature to use that new type and
add the small alias(s) near the top of temoa/extensions/get_comm_tech.py so
callers get proper type information without changing runtime behavior.
In `@temoa/extensions/method_of_morris/morris_sequencer.py`:
- Around line 88-110: The presence checks for numeric Morris inputs currently
treat falsy values (0 or 0.0) as missing; change the checks to explicit "is not
None" when reading morris_inputs for 'perturbation', 'levels', and
'trajectories' so values like 0 or 0.0 are accepted as provided, then separately
validate ranges/constraints (e.g., ensure mm_perturbation is within (0,1],
num_levels is an even positive int > 0 if required, and trajectories is a
positive int) and log/warn or raise on invalid values; update assignments to set
self.mm_perturbation, self.num_levels, and self.trajectories from morris_inputs
only when the key is not None and otherwise fall back to the existing defaults
and log appropriately.
In `@temoa/extensions/method_of_morris/morris.py`:
- Around line 155-159: The variable name Morris_Objectives is using PascalCase
while the rest of the module uses snake_case; rename Morris_Objectives to
morris_objectives consistently where it is assigned and used (the Parallel(...)
call and the subsequent array(...) assignment) so it matches other variables
like y_of and si_of; ensure you update any downstream references to
morris_objectives in this file (e.g., where evaluate(param_names,
param_values[i, :], data, i) results are consumed).
In
`@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py`:
- Around line 258-264: Rename the local variable named vars to avoid shadowing
the built-in; change it to variables (or obj_vars) wherever it's used in this
function (the variable assigned from self.var_vector(model) and subsequently
used in the expr comprehension), update the generator expression that zips vars,
coeffs to zip(variables, coeffs, strict=False), and ensure any other references
in this method (e.g., the sum/expr computation and any asserts) are updated
accordingly to preserve behavior.
In `@temoa/extensions/monte_carlo/mc_run.py`:
- Around line 224-240: Add an explicit return type annotation "-> None" to the
MCRunFactory.__init__ method signature; locate the __init__ in class
MCRunFactory (the constructor that sets self.config, self.data_store,
self.tweak_factory and validates config.monte_carlo_inputs and run_settings and
assigns self.settings_file) and update its definition to include "-> None" while
leaving all existing validation logic and error messages unchanged.
In `@temoa/extensions/monte_carlo/mc_worker.py`:
- Around line 46-56: The __init__ method in mc_worker.py is missing an explicit
return type annotation; update the __init__ signature (the method named __init__
in that file) to include "-> None" as its return type per ANN204, e.g., change
the existing def __init__(...) to def __init__(...) -> None while keeping all
parameters (dp_queue, results_queue, log_root_name, log_queue, solver_name,
solver_options, log_level, solver_log_path) unchanged.
In `@temoa/extensions/myopic/evolution_updater.py`:
- Around line 31-32: The code currently uses assert idx is not None before
accessing idx.base_year which is unsafe because asserts can be stripped; update
the runtime guard in the function that accepts idx: MyopicIndex | None (the
parameter named idx) to explicitly check for None and raise a clear exception
(e.g., ValueError or TypeError) if idx is None, or alternatively remove the None
from the type signature so idx is non-nullable; ensure the logger.info call
(logger.info(f"Running myopic iteration updater for base year {idx.base_year}"))
only runs after the explicit None-check and that the raised exception message
names the parameter (idx) and expected type (MyopicIndex).
In `@temoa/extensions/myopic/myopic_sequencer.py`:
- Around line 706-712: The parameter mi (type MyopicIndex) in
report_total_demand is never used; either use it for the intended logic or mark
it as intentionally unused by renaming to _mi (or remove it from the signature).
Update the function definition report_total_demand(self, mi: MyopicIndex) to
either accept _mi: MyopicIndex if the parameter must remain or remove the
parameter and adjust all callers that pass a MyopicIndex, ensuring the function
body (cursor.execute(...)) and assertions remain unchanged.
- Around line 735-741: The parameter mi in report_cumulative_capacity is unused
(same as report_total_demand); either remove mi from the method signature and
update any callers to stop passing a MyopicIndex, or if the signature must
remain for interface compatibility, mark it as intentionally unused by renaming
to _mi or by adding a single-line use (e.g., "_ = mi" or "del mi") and a brief
comment; adjust the report_cumulative_capacity definition accordingly so the
linter stops flagging an unused parameter.
- Around line 324-331: get_current_total_cost declares a base_year parameter but
never uses it; either make its non-use explicit by renaming it to _base_year in
the method signature (and update any callers) or actually apply it to the SQL
filter: modify the query in get_current_total_cost to include a year condition
(e.g., add an AND year == base_year clause and pass base_year as a parameter to
the execute call) while still asserting self.output_con and self.config as done
now; reference get_current_total_cost, self.output_con.execute, and
self.config.scenario to locate the change.
In `@temoa/extensions/single_vector_mga/output_summary.py`:
- Around line 27-29: Compute total_delta only when orig_cost != 0.0 (same guard
used for per-row percentages) to avoid ZeroDivisionError: if orig_cost is zero,
set total_delta to 0.0 (or the same fallback used for row deltas) before
appending the ['Cost','Total Cost', orig_cost, option_cost, total_delta] row;
update the logic around total_delta, orig_cost and option_cost in
output_summary.py to mirror the per-row percentage guard so the summary step
never divides by zero.
---
Outside diff comments:
In `@temoa/extensions/monte_carlo/mc_run.py`:
- Around line 175-185: The __init__ method of MCRun is missing an explicit
return type annotation; update the MCRun.__init__ signature to include -> None
(i.e., def __init__(self, scenario_name: str, run_index: int, data_store:
dict[str, Any], included_tweaks: dict[Tweak, list[ChangeRecord]]) -> None:) to
make the constructor's return type explicit and satisfy type checkers; keep the
body unchanged.
- Around line 43-54: The __init__ method of the Tweak class is missing an
explicit return annotation; update the Tweak.__init__ signature to include "->
None" (i.e., def __init__(self, param_name: str, indices: tuple[Any, ...],
adjustment: str, value: float) -> None:) so the constructor has the proper
return type annotation and matches typing expectations.
In `@temoa/extensions/monte_carlo/mc_sequencer.py`:
- Around line 153-156: Remove the unused kwargs dict: the local variable kwargs
created in mc_sequencer.py (containing 'solver_name' and 'solver_options') is
never passed anywhere and is redundant because MCWorker is instantiated with
explicit solver_name and solver_options; delete the kwargs construction to avoid
dead code and ensure only the direct MCWorker(...) call (which uses
self.config.solver_name and self.worker_solver_options) remains.
In `@temoa/extensions/stochastics/scenario_creator.py`:
- Around line 37-50: The try/except currently wraps sqlite3.connect plus
HybridLoader.create_data_dict() and manifest iteration, causing manifest or
data-load errors to be reported as "Failed to connect to database"; restrict the
try to only the connection call or change the error message to reflect loading
base data: e.g., move the with sqlite3.connect(temoa_config.input_database) as
con: into its own try/except (catch connection errors only) and then run
HybridLoader(db_connection=con) and hybrid_loader.create_data_dict(...) outside
that connection-only try, or alternatively update the except message to "Failed
to load base data from {temoa_config.input_database}" so failures in
HybridLoader.create_data_dict and iterating hybrid_loader.manifest are not
misreported; refer to HybridLoader, create_data_dict, and hybrid_loader.manifest
when making the change.
---
Duplicate comments:
In `@temoa/extensions/get_comm_tech.py`:
- Around line 12-19: Replace the broad Exception raises with ValueError for
invalid caller-supplied file arguments: in get_tperiods() change the two raise
Exception(...) calls (the "file type not recognized" branch and the "Please
specify a database" branch) to raise ValueError(...) so callers can handle input
validation errors precisely; also make the analogous change in the similar
validation block around lines 47-54 in this file (replace those raise
Exception(...) occurrences with ValueError(...)).
In `@temoa/extensions/method_of_morris/morris_evaluate.py`:
- Around line 82-91: The code reads total_system_cost into output_query then
blindly indexes output_query[0][0]; add a guard that checks if output_query is
empty before indexing (e.g., in the block where y_of is assigned) and handle the
missing-row case explicitly—either raise a clear RuntimeError indicating no
Objective row for the given scenario_name or set y_of to a sensible default
(0.0) depending on expected behavior; update the logic around output_query and
y_of in morris_evaluate.py to avoid IndexError and include scenario_name in the
error/log for diagnostics.
- Around line 64-66: The code constructs a format template in variable
setting_entry but appends the raw template to log_entry; change it to format the
message before appending so the actual values are logged — e.g., replace
log_entry.append(setting_entry) with formatting using the current variables (for
example: setting_entry % (j, param_name, set_idx_tuple, mm_sample[j]) or an
f-string) right after data[param_name][set_idx_tuple] = mm_sample[j]; ensure the
formatted string (not the template) is appended to log_entry so the worker debug
log shows which parameter changed.
In `@temoa/extensions/method_of_morris/morris_sequencer.py`:
- Around line 120-121: The seed parsing now preserves zero but downstream SALib
calls still use the truthy check "if self.seed else None", which converts 0 back
to None; update every place in morris_sequencer.py that passes the seed
(currently written as "if self.seed else None") to instead use an explicit
None-check (e.g., "self.seed if self.seed is not None else None") so that seed=0
is passed through to SALib; apply the same fix for all occurrences referenced in
the review (the two calls around where SALib sampling/resampling is invoked and
the earlier related check).
In `@temoa/extensions/modeling_to_generate_alternatives/hull.py`:
- Around line 28-33: The __init__ of Hull currently accepts and ignores **kwargs
which hides typos; remove the **kwargs from Hull.__init__ signature (def
__init__(self, points: np.ndarray) -> None:) or replace it with explicit
keyword-only parameters if there are legitimate optional settings (e.g., def
__init__(self, points: np.ndarray, *, tol: float = 1e-9) -> None:); update any
internal references to kwargs in the Hull class (none expected) and run the
provided AST-check script to find call sites that pass keyword args so you can
either update those call sites to use the new explicit parameters or remove the
keywords. Ensure the function docstring and any tests reflect the new signature.
In `@temoa/extensions/modeling_to_generate_alternatives/vector_manager.py`:
- Around line 73-76: The abstract method finalize_tracker in the VectorManager
class currently ends with a bare "pass"; replace that with "raise
NotImplementedError()" so that subclasses that don't implement finalize_tracker
fail loudly and consistently with the other abstract methods in this class;
locate the finalize_tracker method definition and change its body to raise
NotImplementedError() with no additional logic.
In `@temoa/extensions/monte_carlo/example_builds/scenario_analyzer.py`:
- Around line 17-19: The histogram code assumes obj_values_tuple is non-empty,
so compute obj_values_tuple (from obj_values) and guard the empty-result case
before calling plt.hist: if obj_values_tuple is empty, skip plotting or handle
it (e.g., log/return or set a minimum bins value) instead of calling plt.hist
with bins=int(sqrt(len(obj_values_tuple))); update the code around
obj_values_tuple and the plt.hist call to check len(obj_values_tuple) == 0 and
handle that branch safely.
- Around line 4-12: The importlib.resources lookup targets
"data_files.example_dbs" which is not part of the installed "temoa" package so
db_resource cannot be resolved after installation; update the lookup and
packaging so the sample DB is shipped with the temoa package: either move the DB
into the temoa package and declare it as package data in pyproject.toml, then
change the resolver line (db_resource =
resources.files('temoa.data_files.example_dbs') / 'utopia.sqlite') to point at
the temoa package, or implement a fallback that opens the DB from a filesystem
path relative to this module (using Path(__file__).parent) when
resources.files('data_files.example_dbs') fails; ensure the symbol db_resource
(and the with resources.as_file(...) as db_path, Connection(...) usage) uses the
corrected resource path so Connection(str(db_path)) will work post-install.
In `@temoa/extensions/single_vector_mga/sv_mga_sequencer.py`:
- Around line 52-53: Normalize and validate svmga_inputs in the class __init__:
parse svmga_inputs = config.svmga_inputs or {}, convert and store typed
attributes (e.g., self.svmga_inputs as dict, self.cost_epsilon: float =
float(svmga_inputs.get('cost_epsilon', 0.05))) and validate/normalize label
lists (e.g., self.start_labels, self.end_labels, self.exact_match_labels) to
ensure they are List[str] (coerce/split/raise on bad types), then remove inline
cast(...) and type: ignore[arg-type] usages in methods like
sv_mga_sequencer.__init__ and the block around lines 125-133 so those methods
consume the already-validated self attributes rather than raw config values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a6ba89df-6d2a-492a-868b-fb448e33ebb5
📒 Files selected for processing (25)
pyproject.tomltemoa/extensions/get_comm_tech.pytemoa/extensions/method_of_morris/morris.pytemoa/extensions/method_of_morris/morris_evaluate.pytemoa/extensions/method_of_morris/morris_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/hull.pytemoa/extensions/modeling_to_generate_alternatives/manager_factory.pytemoa/extensions/modeling_to_generate_alternatives/mga_sequencer.pytemoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/vector_manager.pytemoa/extensions/modeling_to_generate_alternatives/worker.pytemoa/extensions/monte_carlo/example_builds/scenario_analyzer.pytemoa/extensions/monte_carlo/example_builds/scenario_maker.pytemoa/extensions/monte_carlo/mc_run.pytemoa/extensions/monte_carlo/mc_sequencer.pytemoa/extensions/monte_carlo/mc_worker.pytemoa/extensions/myopic/evolution_updater.pytemoa/extensions/myopic/myopic_index.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/extensions/single_vector_mga/output_summary.pytemoa/extensions/single_vector_mga/sv_mga_sequencer.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.py
| def get_info(inputs: dict[str, str]) -> Any: | ||
| inp_file: str | None = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
get_info() still exports Any.
This dispatcher only returns period lists or string-keyed mappings, so Any throws away most of the benefit of the typing added in this PR. A small union or alias here would keep callers type-safe without changing runtime behavior.
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 263-338: Missing explicit return at the end of function able to return non-None value
Add explicit return statement
(RET503)
[warning] 263-263: Too many branches (22 > 12)
(PLR0912)
[warning] 263-263: Dynamically typed expressions (typing.Any) are disallowed in get_info
(ANN401)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/get_comm_tech.py` around lines 263 - 264, get_info currently
returns typing.Any; constrain it to the actual shapes returned (period lists or
string-keyed mappings) by replacing Any with a precise union or alias (e.g.,
define PeriodList = list[str] and PeriodMap = dict[str, PeriodList] and use ->
PeriodList | PeriodMap or a single alias like InfoResult). Update the get_info
signature to use that new type and add the small alias(s) near the top of
temoa/extensions/get_comm_tech.py so callers get proper type information without
changing runtime behavior.
| pert = morris_inputs.get('perturbation') | ||
| if pert: | ||
| self.mm_perturbation = pert | ||
| self.mm_perturbation = float(cast(str | float, pert)) | ||
| logger.info('Morris perturbation: %0.2f', self.mm_perturbation) | ||
| else: | ||
| self.mm_perturbation = 0.10 | ||
| logger.warning( | ||
| 'No value received for perturbation, using default: %0.2f', self.mm_perturbation | ||
| ) | ||
|
|
||
| levels = config.morris_inputs.get('levels') | ||
| levels = morris_inputs.get('levels') | ||
| if levels: | ||
| self.num_levels = levels | ||
| self.num_levels = int(cast('Any', levels)) | ||
| logger.info('Morris levels: %d', self.num_levels) | ||
| else: | ||
| self.num_levels = ( | ||
| 8 # the number of levels to divide the param range into (must be even number) | ||
| ) | ||
| logger.warning('No value received for levels, using default: %d', self.num_levels) | ||
|
|
||
| traj = config.morris_inputs.get('trajectories') | ||
| traj = morris_inputs.get('trajectories') | ||
| if traj: | ||
| self.trajectories = traj | ||
| self.trajectories = int(cast('Any', traj)) |
There was a problem hiding this comment.
Don’t treat numeric Morris settings as “missing.”
perturbation = 0.0 currently falls back to 0.10, and levels = 0 / trajectories = 0 are silently treated as absent instead of explicit input. Use is not None for presence checks here, then validate ranges separately if zero is unsupported.
🧩 Minimal fix
- if pert:
- self.mm_perturbation = float(cast(str | float, pert))
+ if pert is not None:
+ self.mm_perturbation = float(pert)
logger.info('Morris perturbation: %0.2f', self.mm_perturbation)
else:
self.mm_perturbation = 0.10
@@
- if levels:
- self.num_levels = int(cast('Any', levels))
+ if levels is not None:
+ self.num_levels = int(levels)
logger.info('Morris levels: %d', self.num_levels)
else:
self.num_levels = (
@@
- if traj:
- self.trajectories = int(cast('Any', traj))
+ if traj is not None:
+ self.trajectories = int(traj)
logger.info('Morris trajectories: %d', self.trajectories)
else:
self.trajectories = 4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/method_of_morris/morris_sequencer.py` around lines 88 - 110,
The presence checks for numeric Morris inputs currently treat falsy values (0 or
0.0) as missing; change the checks to explicit "is not None" when reading
morris_inputs for 'perturbation', 'levels', and 'trajectories' so values like 0
or 0.0 are accepted as provided, then separately validate ranges/constraints
(e.g., ensure mm_perturbation is within (0,1], num_levels is an even positive
int > 0 if required, and trajectories is a positive int) and log/warn or raise
on invalid values; update assignments to set self.mm_perturbation,
self.num_levels, and self.trajectories from morris_inputs only when the key is
not None and otherwise fall back to the existing defaults and log appropriately.
| Morris_Objectives = Parallel(n_jobs=num_cores)( | ||
| delayed(evaluate)(param_names, param_values[i, :], data, i) for i in range(0, n) | ||
| ) | ||
| ) | ||
| import csv | ||
|
|
||
| line1 = Si_OF['mu_star'] | ||
| line2 = Si_OF['mu_star_conf'] | ||
| line3 = Si_CumulativeCO2['mu_star'] | ||
| line4 = Si_CumulativeCO2['mu_star_conf'] | ||
| with open('MMResults.csv', 'w') as f: | ||
| writer = csv.writer(f, delimiter=',') | ||
| writer.writerow(unique_group_names) | ||
| writer.writerow('Objective Function') | ||
| writer.writerow(line1) | ||
| writer.writerow(line2) | ||
| writer.writerow('Cumulative CO2 Emissions') | ||
| writer.writerow(line3) | ||
| writer.writerow(line4) | ||
|
|
||
| f.close | ||
| print('--- %s seconds ---' % (time.time() - start_time)) | ||
| Morris_Objectives = array(Morris_Objectives) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent variable naming: Morris_Objectives should be morris_objectives.
Other variables in this file were renamed to snake_case (y_of, y_cumulative_co2, si_of, si_cumulative_co2), but Morris_Objectives retains PascalCase. Apply consistent naming.
Proposed fix
- Morris_Objectives = Parallel(n_jobs=num_cores)(
+ morris_objectives = Parallel(n_jobs=num_cores)(
delayed(evaluate)(param_names, param_values[i, :], data, i) for i in range(0, n)
)
- Morris_Objectives = array(Morris_Objectives)
- print(Morris_Objectives)
+ morris_objectives = array(morris_objectives)
+ print(morris_objectives)
si_of = morris.analyze(
problem,
param_values,
- Morris_Objectives[:, 0],
+ morris_objectives[:, 0],🧰 Tools
🪛 Ruff (0.15.6)
[warning] 156-156: Unnecessary start argument in range
Remove start argument
(PIE808)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/method_of_morris/morris.py` around lines 155 - 159, The
variable name Morris_Objectives is using PascalCase while the rest of the module
uses snake_case; rename Morris_Objectives to morris_objectives consistently
where it is assigned and used (the Parallel(...) call and the subsequent
array(...) assignment) so it matches other variables like y_of and si_of; ensure
you update any downstream references to morris_objectives in this file (e.g.,
where evaluate(param_names, param_values[i, :], data, i) results are consumed).
| vars = self.var_vector(model) | ||
|
|
||
| # verify a unit vector | ||
| err = abs(abs(sum(coeffs)) - 1) | ||
| assert err < 1e-6, 'unit vector size error' | ||
| expr = sum(c * v for v, c in zip(vars, coeffs, strict=False) if c != 0) | ||
| return expr |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rename vars to avoid shadowing Python builtin.
The variable name vars shadows the Python builtin function. Rename to variables or obj_vars for clarity.
Proposed fix
# now we need to roll out a vector of the variables and pair them with coefficients...
- vars = self.var_vector(model)
+ variables = self.var_vector(model)
# verify a unit vector
err = abs(abs(sum(coeffs)) - 1)
assert err < 1e-6, 'unit vector size error'
- expr = sum(c * v for v, c in zip(vars, coeffs, strict=False) if c != 0)
- return expr
+ return sum(c * v for v, c in zip(variables, coeffs, strict=False) if c != 0)📝 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.
| vars = self.var_vector(model) | |
| # verify a unit vector | |
| err = abs(abs(sum(coeffs)) - 1) | |
| assert err < 1e-6, 'unit vector size error' | |
| expr = sum(c * v for v, c in zip(vars, coeffs, strict=False) if c != 0) | |
| return expr | |
| variables = self.var_vector(model) | |
| # verify a unit vector | |
| err = abs(abs(sum(coeffs)) - 1) | |
| assert err < 1e-6, 'unit vector size error' | |
| return sum(c * v for v, c in zip(variables, coeffs, strict=False) if c != 0) |
🧰 Tools
🪛 Ruff (0.15.6)
[error] 258-258: Variable vars is shadowing a Python builtin
(A001)
[warning] 264-264: Unnecessary assignment to expr before return statement
Remove unnecessary assignment
(RET504)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@temoa/extensions/modeling_to_generate_alternatives/tech_activity_vector_manager.py`
around lines 258 - 264, Rename the local variable named vars to avoid shadowing
the built-in; change it to variables (or obj_vars) wherever it's used in this
function (the variable assigned from self.var_vector(model) and subsequently
used in the expr comprehension), update the generator expression that zips vars,
coeffs to zip(variables, coeffs, strict=False), and ensure any other references
in this method (e.g., the sum/expr computation and any asserts) are updated
accordingly to preserve behavior.
| def __init__(self, config: TemoaConfig, data_store: dict[str, Any]): | ||
| self.config = config | ||
| self.data_store = data_store | ||
| self.tweak_factory = TweakFactory(data_store) | ||
| self.settings_file = Path(self.config.monte_carlo_inputs['run_settings']) | ||
|
|
||
| def prescreen_input_file(self): | ||
| if not config.monte_carlo_inputs: | ||
| raise ValueError("Monte Carlo mode requires 'monte_carlo_inputs' in the configuration.") | ||
|
|
||
| settings_path = config.monte_carlo_inputs.get('run_settings') | ||
| if not settings_path: | ||
| raise ValueError( | ||
| "Monte Carlo mode requires 'run_settings' path in 'monte_carlo_inputs'." | ||
| ) | ||
|
|
||
| self.settings_file = Path(cast(str, settings_path)) | ||
| if not self.settings_file.exists(): | ||
| raise FileNotFoundError(f'Monte Carlo run settings file not found: {self.settings_file}') |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add -> None return type to MCRunFactory.__init__; validation logic is correct.
The validation for monte_carlo_inputs and run_settings is well implemented with clear error messages. Just add the return type annotation.
Proposed fix
- def __init__(self, config: TemoaConfig, data_store: dict[str, Any]):
+ def __init__(self, config: TemoaConfig, data_store: dict[str, Any]) -> None:📝 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.
| def __init__(self, config: TemoaConfig, data_store: dict[str, Any]): | |
| self.config = config | |
| self.data_store = data_store | |
| self.tweak_factory = TweakFactory(data_store) | |
| self.settings_file = Path(self.config.monte_carlo_inputs['run_settings']) | |
| def prescreen_input_file(self): | |
| if not config.monte_carlo_inputs: | |
| raise ValueError("Monte Carlo mode requires 'monte_carlo_inputs' in the configuration.") | |
| settings_path = config.monte_carlo_inputs.get('run_settings') | |
| if not settings_path: | |
| raise ValueError( | |
| "Monte Carlo mode requires 'run_settings' path in 'monte_carlo_inputs'." | |
| ) | |
| self.settings_file = Path(cast(str, settings_path)) | |
| if not self.settings_file.exists(): | |
| raise FileNotFoundError(f'Monte Carlo run settings file not found: {self.settings_file}') | |
| def __init__(self, config: TemoaConfig, data_store: dict[str, Any]) -> None: | |
| self.config = config | |
| self.data_store = data_store | |
| self.tweak_factory = TweakFactory(data_store) | |
| if not config.monte_carlo_inputs: | |
| raise ValueError("Monte Carlo mode requires 'monte_carlo_inputs' in the configuration.") | |
| settings_path = config.monte_carlo_inputs.get('run_settings') | |
| if not settings_path: | |
| raise ValueError( | |
| "Monte Carlo mode requires 'run_settings' path in 'monte_carlo_inputs'." | |
| ) | |
| self.settings_file = Path(cast(str, settings_path)) | |
| if not self.settings_file.exists(): | |
| raise FileNotFoundError(f'Monte Carlo run settings file not found: {self.settings_file}') |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 224-224: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
[warning] 230-230: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 234-236: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 240-240: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/monte_carlo/mc_run.py` around lines 224 - 240, Add an
explicit return type annotation "-> None" to the MCRunFactory.__init__ method
signature; locate the __init__ in class MCRunFactory (the constructor that sets
self.config, self.data_store, self.tweak_factory and validates
config.monte_carlo_inputs and run_settings and assigns self.settings_file) and
update its definition to include "-> None" while leaving all existing validation
logic and error messages unchanged.
| assert idx is not None | ||
| logger.info(f"Running myopic iteration updater for base year {idx.base_year}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "evolution_updater.py" | head -5Repository: TemoaProject/temoa
Length of output: 108
🏁 Script executed:
cat -n temoa/extensions/myopic/evolution_updater.py | head -60Repository: TemoaProject/temoa
Length of output: 1812
🏁 Script executed:
# Check the function signature and context around line 31
sed -n '1,50p' temoa/extensions/myopic/evolution_updater.py | cat -nRepository: TemoaProject/temoa
Length of output: 1812
🏁 Script executed:
# Search for the function definition that contains these lines
ast-grep --pattern 'function $_ ($_:$_) {
$$$
assert idx is not None
$$$
}'Repository: TemoaProject/temoa
Length of output: 44
🏁 Script executed:
# Use rg to find the function definition
rg -A 20 "def.*iterate" temoa/extensions/myopic/evolution_updater.pyRepository: TemoaProject/temoa
Length of output: 1198
🏁 Script executed:
# Search for how this function is called to understand if None is ever passed
rg "\.iterate\(" temoa/ -A 2 -B 2Repository: TemoaProject/temoa
Length of output: 44
Use a real runtime guard instead of assert for nullable parameter validation.
The function signature allows idx: MyopicIndex | None = None, but assert statements are stripped during optimized Python execution (-O/-OO flags). This means idx.base_year on line 32 can still fail with AttributeError in optimized runs. Either enforce the contract with a real exception or remove None from the type signature.
🛠️ Recommended fix
- assert idx is not None
+ if idx is None:
+ raise ValueError('idx is required')📝 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.
| assert idx is not None | |
| logger.info(f"Running myopic iteration updater for base year {idx.base_year}") | |
| if idx is None: | |
| raise ValueError('idx is required') | |
| logger.info(f"Running myopic iteration updater for base year {idx.base_year}") |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 32-32: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/myopic/evolution_updater.py` around lines 31 - 32, The code
currently uses assert idx is not None before accessing idx.base_year which is
unsafe because asserts can be stripped; update the runtime guard in the function
that accepts idx: MyopicIndex | None (the parameter named idx) to explicitly
check for None and raise a clear exception (e.g., ValueError or TypeError) if
idx is None, or alternatively remove the None from the type signature so idx is
non-nullable; ensure the logger.info call (logger.info(f"Running myopic
iteration updater for base year {idx.base_year}")) only runs after the explicit
None-check and that the raised exception message names the parameter (idx) and
expected type (MyopicIndex).
| def get_current_total_cost(self, base_year: int) -> float: | ||
| assert self.output_con is not None | ||
| assert self.config is not None | ||
| total_cost = self.output_con.execute( | ||
| 'SELECT SUM(d_invest)+SUM(d_fixed)+SUM(d_var)+SUM(d_emiss) FROM output_cost ' | ||
| f'WHERE scenario == "{self.config.scenario}"' | ||
| ).fetchone()[0] | ||
| return float(total_cost) if total_cost is not None else 0.0 |
There was a problem hiding this comment.
Unused parameter base_year in get_current_total_cost.
The base_year parameter is declared but never used in the method body. Either use it to filter results or prefix with underscore to indicate intentional non-use.
Proposed fix (if intentionally unused)
- def get_current_total_cost(self, base_year: int) -> float:
+ def get_current_total_cost(self, _base_year: int) -> float:🧰 Tools
🪛 Ruff (0.15.6)
[warning] 324-324: Unused method argument: base_year
(ARG002)
[error] 328-329: Possible SQL injection vector through string-based query construction
(S608)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/myopic/myopic_sequencer.py` around lines 324 - 331,
get_current_total_cost declares a base_year parameter but never uses it; either
make its non-use explicit by renaming it to _base_year in the method signature
(and update any callers) or actually apply it to the SQL filter: modify the
query in get_current_total_cost to include a year condition (e.g., add an AND
year == base_year clause and pass base_year as a parameter to the execute call)
while still asserting self.output_con and self.config as done now; reference
get_current_total_cost, self.output_con.execute, and self.config.scenario to
locate the change.
| def report_total_demand(self, mi: MyopicIndex) -> None: | ||
| assert self.output_con is not None | ||
| assert self.cursor is not None | ||
| self.cursor.execute( | ||
| "SELECT SUM(demand) FROM output_demand WHERE scenario='original'" | ||
| ) | ||
| self.output_con.commit() |
There was a problem hiding this comment.
Unused parameter mi in report_total_demand.
The mi: MyopicIndex parameter is never used. Either implement the intended functionality using this parameter or prefix with underscore.
Proposed fix (if intentionally unused)
- def report_total_demand(self, mi: MyopicIndex) -> None:
+ def report_total_demand(self, _mi: MyopicIndex) -> None:📝 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.
| def report_total_demand(self, mi: MyopicIndex) -> None: | |
| assert self.output_con is not None | |
| assert self.cursor is not None | |
| self.cursor.execute( | |
| "SELECT SUM(demand) FROM output_demand WHERE scenario='original'" | |
| ) | |
| self.output_con.commit() | |
| def report_total_demand(self, _mi: MyopicIndex) -> None: | |
| assert self.output_con is not None | |
| assert self.cursor is not None | |
| self.cursor.execute( | |
| "SELECT SUM(demand) FROM output_demand WHERE scenario='original'" | |
| ) | |
| self.output_con.commit() |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 706-706: Unused method argument: mi
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/myopic/myopic_sequencer.py` around lines 706 - 712, The
parameter mi (type MyopicIndex) in report_total_demand is never used; either use
it for the intended logic or mark it as intentionally unused by renaming to _mi
(or remove it from the signature). Update the function definition
report_total_demand(self, mi: MyopicIndex) to either accept _mi: MyopicIndex if
the parameter must remain or remove the parameter and adjust all callers that
pass a MyopicIndex, ensuring the function body (cursor.execute(...)) and
assertions remain unchanged.
| def report_cumulative_capacity(self, mi: MyopicIndex) -> None: | ||
| assert self.output_con is not None | ||
| assert self.cursor is not None | ||
| self.cursor.execute( | ||
| "SELECT SUM(capacity) FROM output_capacity WHERE scenario='original'" | ||
| ) | ||
| self.output_con.commit() |
There was a problem hiding this comment.
Unused parameter mi in report_cumulative_capacity.
Same issue as report_total_demand—the mi parameter is unused.
Proposed fix (if intentionally unused)
- def report_cumulative_capacity(self, mi: MyopicIndex) -> None:
+ def report_cumulative_capacity(self, _mi: MyopicIndex) -> None:📝 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.
| def report_cumulative_capacity(self, mi: MyopicIndex) -> None: | |
| assert self.output_con is not None | |
| assert self.cursor is not None | |
| self.cursor.execute( | |
| "SELECT SUM(capacity) FROM output_capacity WHERE scenario='original'" | |
| ) | |
| self.output_con.commit() | |
| def report_cumulative_capacity(self, _mi: MyopicIndex) -> None: | |
| assert self.output_con is not None | |
| assert self.cursor is not None | |
| self.cursor.execute( | |
| "SELECT SUM(capacity) FROM output_capacity WHERE scenario='original'" | |
| ) | |
| self.output_con.commit() |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 735-735: Unused method argument: mi
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/myopic/myopic_sequencer.py` around lines 735 - 741, The
parameter mi in report_cumulative_capacity is unused (same as
report_total_demand); either remove mi from the method signature and update any
callers to stop passing a MyopicIndex, or if the signature must remain for
interface compatibility, mark it as intentionally unused by renaming to _mi or
by adding a single-line use (e.g., "_ = mi" or "del mi") and a brief comment;
adjust the report_cumulative_capacity definition accordingly so the linter stops
flagging an unused parameter.
| records: list[list[Any]] = [['Category', 'Label', 'Original', 'Option', 'Delta [%]']] | ||
| total_delta = (option_cost - orig_cost) / orig_cost * 100 | ||
| records.append(['Cost', 'Total Cost', orig_cost, option_cost, total_delta]) |
There was a problem hiding this comment.
Guard total_delta the same way as the row deltas.
A zero-cost baseline still triggers ZeroDivisionError on Line 28, so the summary step can fail after a successful solve. Use the same orig_cost != 0.0 guard you already use for per-row percentages.
Suggested fix
- total_delta = (option_cost - orig_cost) / orig_cost * 100
+ total_delta = (option_cost - orig_cost) / orig_cost * 100 if orig_cost != 0.0 else None📝 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.
| records: list[list[Any]] = [['Category', 'Label', 'Original', 'Option', 'Delta [%]']] | |
| total_delta = (option_cost - orig_cost) / orig_cost * 100 | |
| records.append(['Cost', 'Total Cost', orig_cost, option_cost, total_delta]) | |
| records: list[list[Any]] = [['Category', 'Label', 'Original', 'Option', 'Delta [%]']] | |
| total_delta = (option_cost - orig_cost) / orig_cost * 100 if orig_cost != 0.0 else None | |
| records.append(['Cost', 'Total Cost', orig_cost, option_cost, total_delta]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/single_vector_mga/output_summary.py` around lines 27 - 29,
Compute total_delta only when orig_cost != 0.0 (same guard used for per-row
percentages) to avoid ZeroDivisionError: if orig_cost is zero, set total_delta
to 0.0 (or the same fallback used for row deltas) before appending the
['Cost','Total Cost', orig_cost, option_cost, total_delta] row; update the logic
around total_delta, orig_cost and option_cost in output_summary.py to mirror the
per-row percentage guard so the summary step never divides by zero.
ParticularlyPythonicBS
force-pushed
the
typing/extensions
branch
from
2340732 to
6a0ef52
Compare
1 participant
Adds typing for all extensions .
Summary by CodeRabbit