Temoa v4 stochastics implementation #246
Conversation
WalkthroughAdds first-class stochastic support: new TOML-parsed dataclasses, a scenario_creator that loads and perturbs DB data, and a StochasticSequencer that runs mpi-sppy ExtensiveForm solves; integrates a STOCHASTIC mode and tests; removes many legacy stochastic scripts and adds mpi-sppy as a dependency and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TemoaSequencer
participant StochasticSequencer
participant mpi_sppy as mpi-sppy
participant ScenarioCreator
participant Database
User->>TemoaSequencer: start(config mode=STOCHASTIC)
TemoaSequencer->>StochasticSequencer: instantiate(config)
StochasticSequencer->>StochasticSequencer: load StochasticConfig.from_toml()
StochasticSequencer->>StochasticSequencer: build options & scenario list
StochasticSequencer->>mpi_sppy: init ExtensiveForm(options, scenario_creator)
loop for each scenario
mpi_sppy->>ScenarioCreator: scenario_creator(name, temoa_config, stoch_config)
ScenarioCreator->>Database: load base tables via HybridLoader
ScenarioCreator->>ScenarioCreator: apply perturbations (filter/action)
ScenarioCreator->>mpi_sppy: return Pyomo ConcreteModel (probability, first-stage info)
end
mpi_sppy->>mpi_sppy: solve extensive form
mpi_sppy->>StochasticSequencer: return objective (expected value)
StochasticSequencer-->>User: expose/store stochastic result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ParticularlyPythonicBS
changed the title
ParticularlyPythonicBS
force-pushed
the
feat/stochastics
branch
from
47798fd to
0cc5c34
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In @pyproject.toml:
- Line 33: Update the dependency spec for mpi-sppy in pyproject.toml from
"mpi-sppy>=0.12.1" to "mpi-sppy>=0.13.1" (replace the existing version token),
then run your test suite and CI to validate compatibility; review the mpi-sppy
changelog between 0.12.1 and 0.13.1 for any API or behavior changes that may
require code updates in places that import or call mpi-sppy (search for
imports/usages of mpi_sppy in the repo) and adjust code accordingly if tests or
changelog indicate breaking or behavioral changes.
In @temoa/extensions/stochastics/scenario_creator.py:
- Line 82: The code constructs periods with sorted(list(instance.time_optimize))
which unnecessarily wraps the iterable in list(); change the expression to
sorted(instance.time_optimize) inside the function where periods is defined (the
variable periods and the attribute instance.time_optimize are the targets) to
remove the redundant list() call while preserving behavior.
- Around line 30-39: Replace the f-string in the logger.warning call inside the
perturbation loop so it uses lazy logging formatting; locate the loop over
stoch_config.perturbations in scenario_creator.py (the variable p and the check
target_param is None) and change the logging invocation to pass a format string
with placeholders and the variables p.table and scenario_name as arguments
(e.g., use "Table %s not found in data_dict for scenario %s" with p.table and
scenario_name) to avoid constructing the message when the warning level is
disabled.
- Around line 81-87: The probability lookup uses a nested dict accessor but
scenarios are a flat mapping; replace the nested .get usage with a flat lookup:
use stoch_config.scenarios.get(scenario_name, 1.0) to set
instance._mpisppy_probability, and update the StochasticConfig type hint
(stochastic_config.py line where scenarios is declared) from dict[str, dict[str,
float]] to dict[str, float] so the types match the TOML structure and the
probability values are read correctly.
- Around line 14-28: The scenario_creator function lacks type annotations and
doesn't handle DB connection errors; add type annotations for scenario_name
(str) and kwargs (e.g., **kwargs: Any or use explicit parameters temoa_config:
TemoaConfig, stoch_config: StochasticConfig) and validate presence of
'temoa_config' and 'stoch_config' before use; wrap the sqlite3.connect(...) call
in a try/except to catch sqlite3.Error (or Exception), log a clear message
including temoa_config.input_database and the caught exception, and re-raise or
raise a custom RuntimeError; keep usage of HybridLoader(db_connection=con,
config=temoa_config) and data_dict =
hybrid_loader.create_data_dict(myopic_index=None) unchanged except for the safe
connection handling.
In @temoa/extensions/stochastics/stochastic_config.py:
- Around line 15-18: The scenarios field on the StochasticConfig dataclass is
incorrectly typed as dict[str, dict[str, float]]; change the type hint to
dict[str, float] (scenario_name -> probability) and update its comment
accordingly so it matches the TOML loader and the expected structure used
elsewhere (see StochasticConfig.scenarios and related usage in
scenario_creator.py around the scenario handling code).
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 61-66: Remove the unused local variable by calling
ef.solve_extensive_form() without assigning its return (or assign to _ if
preferred) and keep setting self.objective_value from ef.get_objective_value();
replace the f-string logger.info(f"Stochastic Expected Value: {obj_val}") with
lazy logging logger.info("Stochastic Expected Value: %s", obj_val) to avoid
unnecessary string interpolation when logging is disabled—references: the
results variable, ef.solve_extensive_form(), obj_val, self.objective_value, and
logger.info.
- Around line 28-33: The ImportError handling in the top-level import block
currently logs a message and raises a new RuntimeError without chaining; change
it to preserve the original exception context by capturing the ImportError as e
and re-raising with exception chaining (e.g., raise RuntimeError("mpi-sppy not
found") from e) or by re-raising the original ImportError; also consider using
logger.exception instead of logger.error to include the traceback when logging
before the raise. Apply this change around the import block that references
mpisppy.utils.sputils, mpisppy.opt.ef.ExtensiveForm, and the logger used there.
- Around line 19-22: The current loading of the stochastic config
(self.config.stochastic_config -> StochasticConfig.from_toml) lacks validation
and error handling; before calling StochasticConfig.from_toml, verify the
referenced path exists and is readable, and wrap the call in a try/except that
catches FileNotFoundError, toml decoding errors (e.g., toml.TomlDecodeError or
equivalent), and general ValueError/Exception from the parser, rethrowing a
ValueError with a clear message that includes the source path and the underlying
error details so users see whether the file is missing or contains invalid TOML.
- Line 45: The code assumes scenarios exist when building all_scenario_names
from self.stoch_config.scenarios; add a pre-check (before creating
ExtensiveForm) to validate that self.stoch_config.scenarios is non-empty and
raise a clear error (e.g., ValueError or RuntimeError) with an actionable
message like "No scenarios found in stoch_config.scenarios; provide at least one
scenario before constructing the ExtensiveForm" so users get a helpful error
instead of mpi-sppy failing later; reference the all_scenario_names assignment
and the subsequent ExtensiveForm construction when adding this guard.
In @tests/testing_configs/config_utopia_stochastic.toml:
- Around line 13-15: Remove the unused [execution] section: delete the block
containing [execution] and solver = "appsi_highs"; then add save_duals = false
at the top level (same placement as other test configs) so the setting is
actually read by the loader (temoa/core/config.py pops/discards [execution]).
Ensure no other references to solver = "appsi_highs" remain in this config.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
docs/source/computational_implementation.rstdocs/source/index.rstdocs/source/stochastics.rstpyproject.tomlrequirements-dev.txtrequirements.txttemoa/_internal/temoa_sequencer.pytemoa/core/config.pytemoa/core/modes.pytemoa/extensions/README.txttemoa/extensions/stochastics/EVPI.pytemoa/extensions/stochastics/README.txttemoa/extensions/stochastics/ReferenceModel.pytemoa/extensions/stochastics/VSS.pytemoa/extensions/stochastics/generate_scenario_tree-nonhomogenous.pytemoa/extensions/stochastics/generate_scenario_tree.pytemoa/extensions/stochastics/jobTemoa.pbstemoa/extensions/stochastics/legacy_files/ef_writer_script_old.pytemoa/extensions/stochastics/legacy_files/scenariomodels.pytemoa/extensions/stochastics/options/README.txttemoa/extensions/stochastics/options/__init__.pytemoa/extensions/stochastics/options/iew2012-nonhomogenized-markov.pytemoa/extensions/stochastics/options/iew2012.pytemoa/extensions/stochastics/options/utopia_coal_vs_nuc.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytemoa/extensions/stochastics/stochastics_README.txttemoa/extensions/stochastics/temoa_stochastic.pytests/conftest.pytests/test_full_runs.pytests/testing_configs/config_utopia_stochastic.tomltests/testing_data/utopia_stochastic_config.toml
💤 Files with no reviewable changes (16)
- temoa/extensions/stochastics/options/README.txt
- temoa/extensions/README.txt
- temoa/extensions/stochastics/stochastics_README.txt
- temoa/extensions/stochastics/generate_scenario_tree.py
- temoa/extensions/stochastics/legacy_files/ef_writer_script_old.py
- temoa/extensions/stochastics/README.txt
- temoa/extensions/stochastics/EVPI.py
- temoa/extensions/stochastics/options/iew2012-nonhomogenized-markov.py
- temoa/extensions/stochastics/VSS.py
- temoa/extensions/stochastics/temoa_stochastic.py
- temoa/extensions/stochastics/generate_scenario_tree-nonhomogenous.py
- temoa/extensions/stochastics/options/utopia_coal_vs_nuc.py
- temoa/extensions/stochastics/ReferenceModel.py
- temoa/extensions/stochastics/jobTemoa.pbs
- temoa/extensions/stochastics/legacy_files/scenariomodels.py
- temoa/extensions/stochastics/options/iew2012.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/_internal/temoa_sequencer.py
🧬 Code graph analysis (2)
temoa/_internal/temoa_sequencer.py (2)
temoa/extensions/stochastics/stochastic_sequencer.py (1)
StochasticSequencer(12-69)temoa/core/modes.py (1)
TemoaMode(9-20)
temoa/extensions/stochastics/stochastic_sequencer.py (3)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (2)
StochasticConfig(16-38)from_toml(21-38)temoa/extensions/stochastics/scenario_creator.py (1)
scenario_creator(14-101)
🪛 Ruff (0.14.10)
temoa/extensions/stochastics/scenario_creator.py
14-14: Missing type annotation for **kwargs
(ANN003)
37-37: Logging statement uses f-string
(G004)
82-82: Unnecessary list() call within sorted()
Remove the inner list() call
(C414)
temoa/extensions/stochastics/stochastic_sequencer.py
17-17: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
32-32: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
33-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
61-61: Local variable results is assigned to but never used
Remove assignment to unused variable results
(F841)
66-66: Logging statement uses f-string
(G004)
🔇 Additional comments (24)
docs/source/index.rst (1)
9-9: LGTM! Documentation navigation updated correctly.The stochastics entry is properly added to the documentation table of contents, aligning with the new stochastic programming functionality introduced in this PR.
requirements.txt (1)
40-41: LGTM! Dependency additions are consistent and correctly generated.The mpi-sppy dependency and its transitive dependency references are properly included. The autogenerated file correctly reflects the pyproject.toml changes with exact version pinning (==0.12.1) and updated dependency comments.
Also applies to: 52-52, 83-85, 105-105
requirements-dev.txt (1)
74-75: LGTM! Dev requirements correctly updated.The mpi-sppy dependency additions and transitive dependency comment updates are consistent with the main requirements file and properly reflect the pyproject.toml changes for the development environment.
Also applies to: 88-88, 127-129, 157-157
temoa/core/modes.py (1)
20-20: LGTM! Clean enum extension for stochastic mode.The STOCHASTIC mode is properly added with sequential numbering and a clear comment. This cleanly extends the TemoaMode enum to support the new stochastic optimization functionality introduced in this PR.
docs/source/computational_implementation.rst (1)
392-392: LGTM! Documentation update accurately reflects the new stochastic implementation.The removal of the "[!] untested in v4.0" qualifier is appropriate given that this PR introduces comprehensive stochastic programming capabilities with test coverage.
tests/conftest.py (2)
95-95: LGTM! Addition of stochastic test database entry is appropriate.The
Nonesource forutopia_stochastic.sqlitecorrectly indicates that this database will be created without initial data scripts, which aligns with the stochastic testing workflow introduced in this PR.
99-100: LGTM! Conditional handling for None source is correct.The implementation properly handles the
Nonecase by creating an empty list, allowing test databases to be created with only the schema and no initial data scripts. This is essential for supporting stochastic test scenarios.tests/testing_data/utopia_stochastic_config.toml (3)
1-3: LGTM! Scenario definitions are well-structured.The two scenarios with equal probabilities (0.5 each) sum to 1.0, which is mathematically correct for a probability distribution. The descriptive names (
low_coalandhigh_coal) clearly indicate the test case intent.
13-19: LGTM! Perturbation for high coal scenario is well-defined.The second perturbation appropriately increases the cost by 50% (multiply by 1.5) for the
high_coalscenario. Together with the first perturbation (0.5x), this creates a symmetric test case exploring both low and high coal cost scenarios.
5-11: IMPHCO1 technology exists and perturbation configuration is valid.The
IMPHCO1technology is defined in the utopia test database as an imported coal supply technology with cost_variable data, confirming this perturbation configuration will function correctly.temoa/_internal/temoa_sequencer.py (3)
36-36: LGTM! Import statement is correctly placed.The import of
StochasticSequenceris appropriately positioned with other extension sequencer imports and follows the established pattern in this module.
71-71: LGTM! Member initialization follows established patterns.The
stochastic_sequencermember is properly typed and initialized, consistent with how other mode-specific sequencers and results are stored in the class (e.g.,pf_results,pf_solved_instance).
194-196: LGTM! STOCHASTIC mode implementation follows established patterns.The implementation correctly:
- Instantiates
StochasticSequencerwith the config- Stores the sequencer instance for test access (as seen in test_full_runs.py)
- Invokes
start()to execute the stochastic workflowThis follows the same pattern used for other modes like MYOPIC and MGA.
tests/testing_configs/config_utopia_stochastic.toml (2)
1-8: LGTM! Main configuration section is correctly structured.The configuration appropriately sets:
scenario_mode = "stochastic"to activate the new stochastic workflow- Input/output paths to the testing directories
appsi_highsas the solver- Time sequencing consistent with utopia test data
10-11: LGTM! Stochastic configuration reference is correct.The
stochastic_configpath correctly points to the stochastic scenario definitions using a relative path. Based on the AI summary,TemoaConfigincludes path resolution support forstochastic_config, so this should work as intended.tests/test_full_runs.py (3)
8-8: LGTM!The addition of
Anyto the typing imports is appropriate for the new stochastic test's type hints.
37-37: LGTM!The
stochastic_fileslist follows the established pattern for test configuration, consistent withmyopic_filesandmc_files.
127-146: LGTM!The stochastic test implementation is sound:
- Defensive check for
stochastic_sequencerexistence before accessing attributes.- Appropriate use of
pytest.approxwith relative tolerance for numerical comparison.- Consistent with existing test patterns.
The hardcoded expected objective value (34389.9878) is reasonable for regression testing. Consider adding a brief comment explaining the source or derivation of this value for future maintainers.
docs/source/stochastics.rst (1)
1-106: LGTM! Clear and comprehensive documentation.The documentation provides excellent coverage of the stochastic programming feature, including installation, configuration, execution flow, and limitations. The examples are helpful and the structure is well-organized.
temoa/core/config.py (3)
57-57: LGTM!The addition of the
stochastic_configparameter to the__init__signature properly supports the new stochastic mode configuration.
148-148: LGTM!The assignment of
self.stochastic_configcorrectly stores the configuration path for later use.
233-246: LGTM!The stochastic section handling properly:
- Extracts the stochastic configuration from the TOML data
- Resolves the relative path correctly using
config_file.parent- Removes non-top-level sections to prevent unexpected kwargs
The implementation is clean and follows the existing pattern for section handling.
temoa/extensions/stochastics/stochastic_config.py (2)
6-13: LGTM!The
Perturbationdataclass is well-structured with appropriate types and clear field names.
20-38: LGTM with note on documentation inconsistency.The
from_tomlimplementation correctly loads the stochastic configuration from TOML. The method properly handles missing sections with defaults and constructsPerturbationobjects with appropriate field mapping.Note: The code reads
'table'from perturbations (line 31), but the documentation examples use'variable'. This inconsistency has been flagged separately in the documentation review.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @temoa/extensions/stochastics/scenario_creator.py:
- Line 36: The logger.exception call is passing the exception object explicitly
which is redundant; edit the logger.exception invocation that currently reads
logger.exception("Failed to connect to database %s: %s",
temoa_config.input_database, e) so it only formats the database name (e.g.,
logger.exception("Failed to connect to database %s",
temoa_config.input_database)), allowing logging.exception to automatically
include the exception traceback.
- Line 14: Move the "from typing import Any" import so it is grouped with the
other top-level imports (place it near the top of
temoa/extensions/stochastics/scenario_creator.py, e.g., immediately after other
standard/library imports) to keep import ordering consistent; ensure the symbol
Any is still available for use in functions or classes in this module after
relocating the import.
- Around line 78-85: The code in scenario_creator.py silently ignores unknown
perturbation actions when handling p.action in the block that updates
target_param (using idx_tuple, current_val, p.value); add explicit validation of
p.action against allowed_actions = {'multiply','add','set'} and if p.action is
not in that set raise a clear ValueError (or log and raise) that includes the
invalid p.action and identifying info about the perturbation (e.g., parameter
name or index) so misconfigurations fail fast and are easy to debug.
In @temoa/extensions/stochastics/stochastic_config.py:
- Around line 6-13: The Perturbation dataclass defines a column field that is
never used by the scenario matching logic; either remove the unused column
attribute from Perturbation or update the matching code in the scenario creation
routine (the loop that checks perturbation.table and perturbation.filter) to
also honor perturbation.column when deciding if a perturbation applies. If you
remove column, delete the attribute and any code/tests that reference it; if you
implement it, update the matching conditional to check the target column
(Perturbation.column) against the current column context and adjust any
constructors/serializers and tests accordingly.
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 28-32: The exception log call in the try/except around
StochasticConfig.from_toml(sc_path) is redundant: remove the explicit exception
object from the logger.exception call so it relies on logging.exception's
automatic exception capture; update the message to include only sc_path (e.g.,
call logger.exception("Failed to load stochastic config from %s", sc_path)) and
keep re-raising the ValueError as-is; look for the try/except that sets
self.stoch_config and references StochasticConfig.from_toml, sc_path, and
logger.exception to make this change.
- Around line 47-50: Replace the magic numbers in the options dict ("loglevel":
50 if self.config.silent else 10) with the logging module constants
(logging.CRITICAL and logging.DEBUG) and ensure the logging module is imported;
update the options assignment (where "solver": self.config.solver_name is set)
to use "loglevel": logging.CRITICAL if self.config.silent else logging.DEBUG so
intent is explicit.
- Around line 76-81: Capture and validate the SolverResults returned by
ef.solve_extensive_form() instead of discarding it; check that
results.solver.status and results.solver.termination_condition indicate a
successful/optimal solve (or use Pyomo's
check_optimal_termination()/assert_optimal_termination()) before calling
ef.get_objective_value(); only then assign to self.objective_value and call
logger.info, otherwise log or raise an error referencing the non-optimal
termination and avoid using ef.get_objective_value() on a failed solve.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
temoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytests/testing_configs/config_utopia_stochastic.toml
🧰 Additional context used
🧠 Learnings (1)
📚 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/stochastics/stochastic_sequencer.py
🧬 Code graph analysis (2)
temoa/extensions/stochastics/stochastic_sequencer.py (3)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (2)
StochasticConfig(16-44)from_toml(21-44)temoa/extensions/stochastics/scenario_creator.py (1)
scenario_creator(16-110)
temoa/extensions/stochastics/scenario_creator.py (5)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (1)
StochasticConfig(16-44)temoa/data_io/hybrid_loader.py (2)
HybridLoader(70-930)data_portal_from_data(145-155)temoa/_internal/run_actions.py (1)
build_instance(121-155)temoa/components/costs.py (1)
period_cost_rule(316-516)
🪛 Ruff (0.14.10)
temoa/extensions/stochastics/stochastic_sequencer.py
17-17: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Avoid specifying long messages outside the exception class
(TRY003)
26-26: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Redundant exception object included in logging.exception call
(TRY401)
32-32: Avoid specifying long messages outside the exception class
(TRY003)
43-43: Avoid specifying long messages outside the exception class
(TRY003)
53-56: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/scenario_creator.py
16-16: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
25-25: Avoid specifying long messages outside the exception class
(TRY003)
36-36: Redundant exception object included in logging.exception call
(TRY401)
37-37: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.13)
🔇 Additional comments (2)
tests/testing_configs/config_utopia_stochastic.toml (1)
1-12: LGTM! Well-structured test configuration.The stochastic test configuration is complete and properly structured. The relative path for
stochastic_configwill be resolved correctly by the config loading logic.temoa/extensions/stochastics/scenario_creator.py (1)
97-108: No action needed.attach_root_nodefrom mpi-sppy explicitly handles empty variable lists gracefully. Whenfirst_stage_varsis empty (i.e., nov_new_capacityentries for the first period), the node will simply have no first-stage nonanticipative variables, which is valid behavior per mpi-sppy's design.
ParticularlyPythonicBS
force-pushed
the
feat/stochastics
branch
from
2c9623e to
24dc591
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @temoa/extensions/stochastics/scenario_creator.py:
- Around line 73-83: The code recreates allowed_actions inside the perturbation
loop; move the set declaration out of the loop to avoid repeated allocation:
define allowed_actions = {'multiply', 'add', 'set'} once before the loop that
iterates perturbations (the loop using variable p) and then remove the in-loop
declaration, keeping the subsequent checks that use p.action and the assignments
to target_param[idx_tuple] with current_val unchanged.
- Around line 62-71: The comprehension building index_map can silently truncate
if index_cols and idx_tuple lengths differ; update the comprehension in the loop
over target_param to use zip(index_cols, idx_tuple, strict=True) so mismatched
lengths raise immediately (i.e., change "{col: val for col, val in
zip(index_cols, idx_tuple)}" to use strict=True) and run tests to ensure this
code path handles any length errors; reference symbols: target_param,
index_cols, idx_tuple, index_map, p.filter.
In @temoa/extensions/stochastics/stochastic_config.py:
- Around line 24-30: The scenarios probabilities parsed into the local variable
scenarios should be validated to ensure they form a proper distribution; after
the loop that builds scenarios (the for loop over scenarios_raw in
stochastic_config.py) compute total_prob = sum(scenarios.values()) and warn if
total_prob is outside an acceptable tolerance (e.g., 0.99–1.01). Add
module-level logging (import logging and logger = logging.getLogger(__name__))
and call logger.warning with the sum and a helpful message if the check fails to
alert users to misconfigured probabilities.
- Around line 33-40: The loop that constructs Perturbation objects from
perturbations_data can raise unclear KeyError if a required field is missing; in
the loop around Perturbation(...) (the block creating scenario=, table=,
filter=, value=), validate that keys 'scenario', 'table', 'filter', and 'value'
exist (or wrap the Perturbation construction in try/except KeyError) and raise a
clearer exception (e.g., ValueError) that includes the perturbation index or the
offending dict and the missing key name so callers get actionable context.
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 88-90: Initialize the objective_value attribute in the
StochasticSequencer.__init__ to avoid AttributeError when accessed before
start() or after a failed solve; add an assignment like self.objective_value =
None (typed as float | None if using typing) inside __init__ so the attribute
always exists and callers can check for None before using it.
- Around line 29-33: The except block that catches exceptions from
StochasticConfig.from_toml(sc_path) should chain the original exception to
preserve context: change the handler to capture the exception (e.g., except
Exception as e:) and re-raise the ValueError using "from e" so the original
traceback is retained; keep the existing logger.exception call
(logger.exception("Failed to load stochastic config from %s", sc_path)) and then
raise ValueError(f"Error parsing stochastic config {sc_path}") from e, referring
to StochasticConfig.from_toml and self.stoch_config in stochastic_sequencer.py.
- Around line 8-9: Remove the no-op TYPE_CHECKING block and its unused import:
delete the "if TYPE_CHECKING: pass" block from the module and remove
"TYPE_CHECKING" from the "from typing import ..." import statement (or delete
the import entirely if that line becomes unused); then run the linter/format to
ensure there are no remaining unused-import errors.
- Around line 48-51: The call to solve_extensive_form() in StochasticSequencer
is missing the solver_options argument; use the already-built options dict (or
load solver options from the stochastic config like the Monte Carlo/MGA
extensions do) and pass it as solver_options=options when calling
solve_extensive_form(); ensure the config field name matches (e.g.,
self.config.solver_options or similar) and merge any default keys (solver,
loglevel) into that dict before passing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
temoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytests/testing_configs/config_utopia_stochastic.toml
🧰 Additional context used
🧠 Learnings (1)
📚 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/stochastics/stochastic_sequencer.pytemoa/extensions/stochastics/scenario_creator.py
🪛 Ruff (0.14.10)
temoa/extensions/stochastics/stochastic_sequencer.py
18-18: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
21-21: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
33-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Avoid specifying long messages outside the exception class
(TRY003)
54-57: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/scenario_creator.py
15-15: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
24-24: Avoid specifying long messages outside the exception class
(TRY003)
43-43: Avoid specifying long messages outside the exception class
(TRY003)
64-64: Unnecessary dict comprehension (rewrite using dict())
Rewrite using dict()
(C416)
64-64: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (windows-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 (macos-latest, 3.13)
🔇 Additional comments (3)
tests/testing_configs/config_utopia_stochastic.toml (1)
1-12: LGTM!The stochastic test configuration is well-structured. The relative path for
stochastic_configis appropriate for the test directory layout, and all necessary fields for stochastic mode are present.temoa/extensions/stochastics/scenario_creator.py (1)
15-24: Well-structured mpi-sppy integration.The scenario creator correctly implements the mpi-sppy callback pattern: loading base data, applying scenario-specific perturbations, building the Pyomo instance, and attaching the root node with first-stage variables and cost. The identification of first-stage decisions (capacity investments in the first period) aligns with standard stochastic programming practice.
Also applies to: 85-107
temoa/extensions/stochastics/stochastic_sequencer.py (1)
13-16: Good orchestration of the stochastic workflow.The
StochasticSequencercleanly separates configuration validation in__init__from execution instart(). The dynamic import pattern for mpi-sppy allows graceful handling when the optional dependency is missing. The ExtensiveForm construction follows the expected mpi-sppy API.Also applies to: 35-38, 68-77
ParticularlyPythonicBS
force-pushed
the
feat/stochastics
branch
from
24dc591 to
040da8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 49-54: The current shallow update of the options dict (the
variables options and solver_options in stochastic_sequencer.py) will overwrite
nested dicts; implement a deep merge instead by adding a helper (e.g.,
deep_merge) that recursively merges nested dicts and then replace
options.update(solver_options) with options = deep_merge(options,
solver_options); ensure the helper preserves non-dict values from override and
returns a new dict so existing references to options are not mutated
unexpectedly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
temoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytests/testing_configs/config_utopia_stochastic.toml
🧰 Additional context used
🧠 Learnings (1)
📚 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/stochastics/stochastic_sequencer.pytemoa/extensions/stochastics/scenario_creator.py
🧬 Code graph analysis (1)
temoa/extensions/stochastics/stochastic_sequencer.py (3)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (2)
StochasticConfig(20-69)from_toml(26-69)temoa/extensions/stochastics/scenario_creator.py (1)
scenario_creator(19-119)
🪛 Ruff (0.14.10)
temoa/extensions/stochastics/stochastic_config.py
61-61: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/stochastic_sequencer.py
16-16: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
24-24: Avoid specifying long messages outside the exception class
(TRY003)
26-26: Avoid specifying long messages outside the exception class
(TRY003)
32-32: Avoid specifying long messages outside the exception class
(TRY003)
42-42: Avoid specifying long messages outside the exception class
(TRY003)
57-60: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/scenario_creator.py
19-19: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
28-28: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (12)
tests/testing_configs/config_utopia_stochastic.toml (1)
1-12: LGTM! Configuration is well-structured.The stochastic configuration file is properly structured with all necessary fields for a stochastic run, including the appropriate solver and path to the stochastic configuration.
temoa/extensions/stochastics/stochastic_config.py (3)
1-8: LGTM! Clean imports.The imports are appropriate and well-organized for parsing TOML configuration files.
10-23: LGTM! Well-defined dataclasses.The dataclass definitions appropriately model the stochastic configuration structure with clear field types and sensible defaults.
25-69: LGTM! Robust TOML parsing with good error handling.The
from_tomlmethod correctly:
- Handles both dictionary and direct float probability specifications
- Validates that probabilities sum to approximately 1.0 with reasonable tolerance
- Provides clear error messages for missing required fields in perturbations
- Defaults the action field to 'set' when not specified
The static analysis hint about TRY003 (long exception messages) can be safely ignored here, as the descriptive error messages aid debugging.
temoa/extensions/stochastics/stochastic_sequencer.py (2)
16-33: LGTM! Thorough configuration validation.The
__init__method properly validates the stochastic configuration:
- Ensures
stochastic_configis provided- Validates the path exists and is a file
- Handles loading errors with clear context
The detailed error messages (flagged by static analysis TRY003) are appropriate for configuration validation scenarios.
34-96: LGTM! Well-structured stochastic solver orchestration.The
startmethod correctly:
- Lazily imports mpi-sppy with appropriate error handling
- Merges solver options from both config and stochastic config
- Validates that at least one scenario exists before constructing ExtensiveForm
- Checks for optimal termination before accessing objective value
- Extracts and stores the objective value for downstream use
The TODO for TableWriter integration is appropriately noted for future work.
temoa/extensions/stochastics/scenario_creator.py (6)
1-31: LGTM! Proper mpi-sppy scenario creator interface.The function signature correctly implements the mpi-sppy scenario creator pattern:
- Uses
**kwargsto receive framework-provided arguments (theAnytype hint is appropriate here)- Validates required configuration objects are present
- Properly types the extracted configurations for downstream use
33-47: LGTM! Proper data loading with good error handling.The base data loading correctly:
- Uses context manager for database connection
- Calls
HybridLoader.create_data_dictwithmyopic_index=None(appropriate for stochastic mode)- Builds the table index map from the manifest for perturbation matching
- Handles connection and loading errors with clear context
The index column extraction (
item.columns[:-1]) assumes tables have at least one index column, which aligns with the data model design.
49-75: LGTM! Robust perturbation validation.The perturbation setup properly:
- Defines allowed actions that match the stochastic config documentation
- Filters perturbations by scenario name
- Logs warnings for missing tables or manifest entries (allowing partial perturbations)
- Validates actions strictly, raising clear errors for invalid operations
76-94: LGTM! Excellent perturbation application logic.The perturbation application is well-implemented:
- Creates a list copy for safe iteration while modifying the dictionary
- Uses
strict=Trueinzip()(line 78) to ensure index columns and tuple lengths match—excellent defensive programming!- Implements clear filter matching logic
- Correctly applies multiply, add, and set operations
95-119: LGTM! Correct mpi-sppy integration.The instance building and root node attachment properly:
- Converts the modified data dictionary to a Pyomo data portal
- Builds the Temoa model instance silently (appropriate for scenario creation)
- Identifies the first period and sets the scenario probability
- Collects first-stage variables (new capacity decisions for the first period)
- Calculates first-stage cost using Temoa's
period_cost_rule- Attaches the root node with correct arguments for mpi-sppy
106-110: Verify behavior when no first-stage capacity decisions exist.The code collects first-stage variables by filtering
v_new_capacityfor the first period. If a scenario has no new capacity decisions in the first period,first_stage_varswould be empty.Please verify whether:
- This is a valid scenario configuration (some models may not require first-period investments)
- This should be treated as an error condition requiring validation
ParticularlyPythonicBS
force-pushed
the
feat/stochastics
branch
from
040da8c to
3da806e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @temoa/extensions/stochastics/scenario_creator.py:
- Around line 73-74: The check for p.action against allowed_actions in
scenario_creator.py is redundant because Perturbation.__post_init__ in
stochastic_config.py already validates actions; remove the conditional that
raises ValueError (the p.action not in allowed_actions branch) and also remove
the now-unused allowed_actions definition referenced on line 50 so validation
relies on Perturbation.__post_init__ only.
In @temoa/extensions/stochastics/stochastic_config.py:
- Around line 54-68: Perturbations may reference nonexistent scenarios so update
the loop that builds perturbations (perturbations_data -> perturbations,
constructing Perturbation) to validate that p['scenario'] exists in the
scenarios mapping before appending; if p['scenario'] is missing or not a key in
scenarios, raise a ValueError identifying the perturbation index and the invalid
scenario name to fail fast and avoid silent ignores. Ensure you still catch
KeyError for other missing fields as before but perform the scenario membership
check (using the scenarios dict) prior to creating the Perturbation.
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 85-92: The call to ef.solve_extensive_form is passing the full
options dict instead of only solver-specific settings; change the argument to
use the previously extracted solver_options variable (used earlier on line 59)
so that ef.solve_extensive_form receives only solver-specific params; update the
call where ef (ExtensiveForm instance) invokes solve_extensive_form to pass
solver_options rather than options.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
docs/source/stochastics.rsttemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytests/testing_configs/config_utopia_stochastic.toml
🧰 Additional context used
🧠 Learnings (1)
📚 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/stochastics/stochastic_sequencer.pytemoa/extensions/stochastics/scenario_creator.py
🧬 Code graph analysis (2)
temoa/extensions/stochastics/stochastic_sequencer.py (3)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (2)
StochasticConfig(27-76)from_toml(33-76)temoa/extensions/stochastics/scenario_creator.py (1)
scenario_creator(19-134)
temoa/extensions/stochastics/scenario_creator.py (5)
temoa/_internal/run_actions.py (1)
build_instance(121-155)temoa/components/costs.py (1)
period_cost_rule(316-516)temoa/data_io/hybrid_loader.py (1)
data_portal_from_data(145-155)temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (1)
StochasticConfig(27-76)
🪛 Ruff (0.14.10)
temoa/extensions/stochastics/stochastic_sequencer.py
17-17: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
21-21: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
69-72: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/scenario_creator.py
19-19: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
28-28: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/stochastic_config.py
21-23: Avoid specifying long messages outside the exception class
(TRY003)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
🔇 Additional comments (9)
tests/testing_configs/config_utopia_stochastic.toml (1)
1-12: LGTM!The stochastic test configuration is well-structured. The relative path for
stochastic_configcorrectly points to the siblingtesting_datadirectory, and the configuration options are appropriate for a stochastic test run.docs/source/stochastics.rst (1)
1-107: LGTM!The documentation is comprehensive and accurately reflects the implementation. It clearly explains the configuration structure, perturbation options, and workflow. The limitations section appropriately sets expectations about current capabilities.
temoa/extensions/stochastics/scenario_creator.py (3)
19-28: LGTM!The function signature and kwargs validation are well-implemented. The early validation of required configuration objects ensures clear error messages if the function is called incorrectly.
112-125: First-stage variables limited tov_new_capacity.The implementation only considers
v_new_capacityvariables for the first period as first-stage decisions. This aligns with the documented two-stage limitation. Consider adding a comment or documentation note if other capacity-related variables (like retirement decisions) should also be included in future extensions.
39-44: Column convention is correct and well-documented.The assumption that the last column in each
LoadItemis always the value (or set element) and preceding columns are indices is explicitly documented in theLoadItemclass docstring: "The last column is assumed to be the value for aParam." This convention is consistently applied across all manifest items—Sets use a single column (index extraction yields an empty list), and Params use the last column as the value with preceding columns as indices. The code correctly implements this withitem.columns[:-1].temoa/extensions/stochastics/stochastic_config.py (1)
10-23: LGTM!The
Perturbationdataclass is well-designed with clear field definitions and early validation in__post_init__. The validation ensures invalid actions fail at parse time rather than during scenario execution.temoa/extensions/stochastics/stochastic_sequencer.py (3)
17-33: LGTM!The
__init__method properly validates the stochastic configuration path and handles errors gracefully. The sequential existence and file-type checks provide clear error messages.
35-44: LGTM!The
_deep_mergehelper correctly handles recursive dictionary merging for nested solver options.
94-105: LGTM!The termination check correctly uses Pyomo's
check_optimal_terminationand extracts the termination condition for error reporting. The objective value retrieval and logging are well-implemented.
ParticularlyPythonicBS
force-pushed
the
feat/stochastics
branch
from
3da806e to
7ebb459
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @temoa/extensions/stochastics/scenario_creator.py:
- Around line 27-28: The scenario_creator function currently requires
'temoa_config' and 'stoch_config' via kwargs and validates them at runtime;
change the function signature to accept explicit typed parameters (e.g.,
scenario_creator(temoa_config: TemoaConfig, stoch_config: StochConfig, *args,
**kwargs)) so callers and IDEs get type safety and clear contracts, update
internal references to use these parameters directly, add a thin shim or
backward-compatible check to still accept kwargs when invoked by mpi-sppy
(preserve compatibility by extracting them from kwargs if present), and update
any callers/tests to pass the new arguments.
- Around line 72-90: Loop over target_param in scenario_creator code currently
handles p.action == 'multiply'|'add'|'set' but lacks a defensive else; update
the block inside the if match branch (the code manipulating target_param
entries) to include an explicit else that handles unexpected p.action
values—e.g., raise a clear ValueError or log and skip—so that unknown actions in
Perturbation are caught at runtime; reference the loop using target_param,
idx_tuple, current_val and the Perturbation instance p to locate where to add
the else.
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 35-44: The _deep_merge function currently mutates the passed-in
base dict in place and also returns it, which can surprise callers; either make
the behavior explicit in the docstring (state that _deep_merge mutates base in
place and returns the same dict) or convert it to a pure function: create a deep
copy of base (e.g., via copy.deepcopy), recursively merge update into that new
dict (mirroring the existing recursion in _deep_merge), and return the new dict
without modifying the original base; if you keep the in-place semantics, remove
the misleading return or keep it but document it clearly in the docstring
referencing the _deep_merge signature and the base/update parameters.
- Around line 17-33: Add an explicit return type annotation to the constructor:
update the __init__ method signature to include "-> None" to match modern Python
typing conventions; locate the __init__ in class StochasticSequencer (the block
that references self.config, self.objective_value, sc_path, and calls
StochasticConfig.from_toml) and add the return type only—no other logic changes.
- Line 63: The current 'loglevel' assignment sets logging.CRITICAL when
self.config.silent is true and logging.DEBUG otherwise; change this to a more
balanced default by using logging.INFO (or logging.WARNING if you prefer less
verbosity) instead of logging.DEBUG when not silent. Locate the dict entry with
the key 'loglevel' (the line containing "'loglevel': logging.CRITICAL if
self.config.silent else logging.DEBUG,") and update the else branch to
logging.INFO (i.e., "'loglevel': logging.CRITICAL if self.config.silent else
logging.INFO,") so normal runs emit informational messages rather than full
debug output.
- Around line 59-66: The code redundantly merges solver-specific settings into
the framework options: remove the self._deep_merge(options, solver_options) call
so the options dict only contains framework-level keys ('solver' and 'loglevel')
and leave solver_options passed exclusively to solve_extensive_form(...). Update
around the construction of options (the variables options, solver_options and
the self._deep_merge call) so ExtensiveForm gets framework options only and
solve_extensive_form is given solver_options unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/source/computational_implementation.rstdocs/source/stochastics.rsttemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytests/testing_configs/config_utopia_stochastic.toml
🧰 Additional context used
🧠 Learnings (1)
📚 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/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_sequencer.py
🧬 Code graph analysis (1)
temoa/extensions/stochastics/stochastic_sequencer.py (3)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (2)
StochasticConfig(27-83)from_toml(33-83)temoa/extensions/stochastics/scenario_creator.py (1)
scenario_creator(19-130)
🪛 Ruff (0.14.10)
temoa/extensions/stochastics/scenario_creator.py
19-19: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
28-28: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/stochastic_config.py
21-23: Avoid specifying long messages outside the exception class
(TRY003)
60-63: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/stochastic_sequencer.py
17-17: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
21-21: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
69-72: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (windows-latest, 3.13)
- 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)
🔇 Additional comments (15)
docs/source/computational_implementation.rst (1)
392-392: LGTM! Documentation reference updated appropriately.The update correctly removes the "untested" qualifier and adds a proper documentation cross-reference to the new stochastics guide, aligning with the stochastic implementation introduced in this PR.
docs/source/stochastics.rst (2)
1-107: Excellent documentation for the stochastics extension!The documentation is comprehensive and well-structured, covering installation, configuration, usage patterns, and current limitations. The TOML examples are clear and the perturbation options are well-documented.
14-24: No changes needed. The installation commands are correct and properly documented.The
uv add mpi-sppysyntax is valid per the official uv package manager documentation. The mpi-sppy package is a legitimate PyPI dependency (currently at version 0.13.1) and is already listed in the project's pyproject.toml. Both installation methods shown (viauv addandpip install) are standard and correct.tests/testing_configs/config_utopia_stochastic.toml (2)
1-12: Test configuration looks appropriate.The configuration properly sets up a stochastic test scenario with reasonable solver and output settings for testing purposes.
11-12: Stochastic config file path is correct and valid.The file
tests/testing_data/utopia_stochastic_config.tomlexists and contains valid TOML configuration with properly defined scenarios and perturbations.temoa/extensions/stochastics/scenario_creator.py (3)
34-47: Excellent error handling for database operations.The try-except block properly catches connection/loading errors, logs context, and re-raises with a helpful message. The context manager ensures proper connection cleanup.
108-122: Good validation of first-stage variables.The code correctly identifies first-stage variables and raises a clear error if none are found, which is essential for stochastic optimization to function properly.
95-129: First-stage identification aligns with mpi-sppy requirements.The implementation correctly identifies
v_new_capacity[*, *, first_period]as first-stage variables, sets_mpisppy_probability, and callsattach_root_node()as required by mpi-sppy for two-stage problems. Since perturbations only modify parameter values (not model structure), all scenarios will have identical variable indices. Consider adding an explicit comment or settingdo_not_check_variable_probabilities=Truein theattach_root_node()call to document this assumption, though the current implementation is functionally correct.temoa/extensions/stochastics/stochastic_config.py (4)
10-23: Excellent validation inPerturbation.__post_init__.The dataclass properly validates the action field against allowed values, preventing invalid actions from being created. This validation complements the action handling in
scenario_creator.py.
45-52: Appropriate floating-point tolerance for probability validation.The 0.99 to 1.01 range accounts for floating-point arithmetic imprecision while still catching significant probability distribution errors. The warning message is helpful for users.
56-75: Robust perturbation validation with clear error messages.The validation properly checks:
- Scenario existence before creating perturbations
- Required fields with helpful KeyError handling
- Index tracking for debugging (line 61 includes index in error message)
The detailed error messages (flagged by static analysis) are appropriate here as they help users diagnose configuration issues.
32-83: Well-designed TOML parser with flexible schema support.The
from_tomlmethod handles:
- Flexible probability formats (float or dict with 'probability' key)
- Sensible defaults (line 70: action defaults to 'set')
- Comprehensive validation with informative error messages
temoa/extensions/stochastics/stochastic_sequencer.py (3)
1-9: LGTM!The imports are well-organized and appropriate for integrating mpi-sppy with Temoa's configuration system.
74-90: LGTM!The ExtensiveForm construction correctly passes scenario names, the creator function, and necessary kwargs for scenario instantiation.
94-105: Robust error handling and termination verification.The code properly validates optimal termination before accessing the objective value and provides detailed error information on failure. The extraction and logging of the stochastic expected value is clean.
ParticularlyPythonicBS
force-pushed
the
feat/stochastics
branch
from
7ebb459 to
b491d93
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @temoa/extensions/stochastics/stochastic_config.py:
- Around line 32-83: The from_toml classmethod currently allows an empty
scenarios dict; add a validation after building scenarios that raises a
ValueError if scenarios is empty (e.g., if not scenarios:) with a clear message
like "No scenarios defined; at least one scenario is required for stochastic
optimization", so callers of StochasticConfig.from_toml will get a clear error
instead of silently proceeding; update the method around the existing "Validate
probability distribution" block and reference the scenarios variable and the
from_toml classmethod in your change.
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 17-33: The constructor __init__ of the StochasticSequencer
validates and loads stochastic_config but should have an explicit return type
and can improve message lengths or use a custom exception; add a -> None
annotation to __init__ and (optionally) replace the long ValueError messages
with either shorter messages or raise a custom StochasticConfigError (define a
new exception class) while keeping the existing checks
(config.stochastic_config, sc_path.exists(), sc_path.is_file()) and keep logging
the original exception when calling StochasticConfig.from_toml(sc_path) via
logger.exception before re-raising.
- Around line 35-43: The _deep_merge method in stochastic_sequencer.py is unused
dead code; remove the entire _deep_merge(self, base: dict[str, Any], update:
dict[str, Any]) method definition from the class, and if that removal leaves any
now-unused imports (e.g., Any from typing), remove those imports as well; ensure
no other code references _deep_merge before deleting.
- Around line 45-105: The options dict passed to ExtensiveForm in start()
currently includes an undocumented 'loglevel' key; remove 'loglevel' from the
options dict construction and only include the documented 'solver' key (use
self.config.solver_name). If you need to control mpi-sppy logging instead,
configure it via mpi-sppy’s documented mechanism or pass logging via
solver_options (already derived from self.stoch_config.solver_options) rather
than adding 'loglevel' to options; update the code that builds options
(referencing options, self.config.silent, and self.config.solver_name) and
ensure only supported keys are passed to ExtensiveForm.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/source/computational_implementation.rstdocs/source/stochastics.rsttemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytests/testing_configs/config_utopia_stochastic.toml
🧰 Additional context used
🧠 Learnings (1)
📚 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/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_sequencer.py
🧬 Code graph analysis (2)
temoa/extensions/stochastics/scenario_creator.py (4)
temoa/_internal/run_actions.py (1)
build_instance(121-155)temoa/components/costs.py (1)
period_cost_rule(316-516)temoa/data_io/hybrid_loader.py (2)
HybridLoader(70-930)data_portal_from_data(145-155)temoa/extensions/stochastics/stochastic_config.py (1)
StochasticConfig(27-83)
temoa/extensions/stochastics/stochastic_sequencer.py (3)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (2)
StochasticConfig(27-83)from_toml(33-83)temoa/extensions/stochastics/scenario_creator.py (1)
scenario_creator(19-130)
🪛 Ruff (0.14.10)
temoa/extensions/stochastics/scenario_creator.py
19-19: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
28-28: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/stochastic_config.py
21-23: Avoid specifying long messages outside the exception class
(TRY003)
60-63: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/stochastic_sequencer.py
17-17: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
21-21: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
66-69: Avoid specifying long messages outside the exception class
(TRY003)
98-98: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.13)
- GitHub Check: setup and test (macos-latest, 3.13)
- GitHub Check: setup and test (windows-latest, 3.12)
🔇 Additional comments (12)
temoa/extensions/stochastics/stochastic_config.py (2)
10-23: LGTM! Clean validation logic.The
Perturbationdataclass is well-structured with appropriate field validation in__post_init__. The action validation ensures only supported operations are used.
26-30: LGTM! Proper use of default factories.The dataclass correctly uses
field(default_factory=...)for mutable default values, following Python best practices.docs/source/computational_implementation.rst (1)
392-392: LGTM! Documentation accurately reflects the new stochastics implementation.The update appropriately removes the "[!] untested in v4.0" qualifier and adds a documentation reference, aligning with the newly implemented and tested stochastic programming capabilities.
docs/source/stochastics.rst (1)
1-107: LGTM! Comprehensive and clear documentation.The stochastics documentation is well-structured and covers all essential aspects:
- Clear dependency installation instructions
- Detailed configuration examples
- Comprehensive perturbation option descriptions
- Transparent workflow explanation
- Honest limitations disclosure
This will help users effectively utilize the new stochastic programming capabilities.
temoa/extensions/stochastics/scenario_creator.py (6)
19-31: LGTM! Appropriate kwargs validation.The function correctly validates required kwargs before proceeding. The use of
typing.Anyfor**kwargsis acceptable here since this is a callback function for mpi-sppy with a flexible signature pattern.
49-90: LGTM! Solid perturbation logic with good defensive programming.The perturbation application is well-implemented:
- Line 72 correctly creates a copy with
list(target_param.items())to avoid modification during iteration- Line 74 uses
strict=Trueinzip()for safety- Proper warning logs for missing tables/manifests
- All three action types are handled
One observation: an empty
p.filterdict would match all entries in the table, which might be intentional for broad perturbations but could also be an error. Consider whether this should be documented or validated.
91-93: LGTM! Clean instance construction.The instance building correctly uses the modified data dictionary and appropriately silences verbose output during scenario creation.
95-107: LGTM! Proper first-stage setup.The code correctly identifies the first period and sets the scenario probability following mpi-sppy conventions. The warning for missing probabilities is good defensive programming.
108-128: Clarify first-stage variable selection: capacity investments vs. all first-period decisions.The code collects only
v_new_capacityvariables with deployment period equal tofirst_periodas first-stage decisions. However, the documentation (stochastics.rst) states: "all decisions in the first time period are considered first-stage."These statements conflict. The current implementation intentionally selects only capacity investments as first-stage, excluding other first-period decision variables such as
v_capacity,v_flow_out,v_retired_capacity, andv_annual_retirement.Note: The loop variable
pinfor r, t, p in instance.v_new_capacityrepresents the deployment vintage (year), not the optimization period—this naming could be clarified.Please confirm whether:
- Only capacity investments should be first-stage (design choice), and documentation should be updated, or
- All first-period decisions should be first-stage (as documentation implies), and the code should be expanded to include other decision variables.
33-47: No actionable issue found. Single-column items in the manifest are Sets (model.regions, techs, commodities), not Parameters. Since perturbations only apply to Parameters, and all Parameters in the manifest have at least 2 columns (index + value), the edge case of an empty index list does not occur in practice. The code is safe as-is.tests/testing_configs/config_utopia_stochastic.toml (1)
1-12: LGTM! Test configuration is well-structured.The stochastic test configuration correctly sets up the scenario mode and references the stochastic config file. The relative path
../testing_data/utopia_stochastic_config.tomlresolves correctly from the config file's directory, and the referenced configuration file exists.temoa/extensions/stochastics/stochastic_sequencer.py (1)
1-9: LGTM: Imports are clean and appropriate.All imports are used and properly organized.
ParticularlyPythonicBS
force-pushed
the
feat/stochastics
branch
from
b491d93 to
53b89e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @temoa/extensions/stochastics/scenario_creator.py:
- Around line 83-89: Add a defensive else branch to the action handling block
that logs and raises on unexpected actions: after the existing if/elif chain
that checks p.action == 'multiply' / 'add' / 'set' (where you modify
target_param[idx_tuple]), add an else that calls logger.error('Invalid action %s
for perturbation', p.action) and raises ValueError(f'Invalid action:
{p.action}'); this complements the existing Perturbation.__post_init__
validation and protects against future changes or bypassed validation.
- Around line 99-106: The code currently defaults missing scenario probabilities
to 1.0 which can break normalization; in scenario_creator.py change the behavior
in the block that retrieves prob from stoch_config.scenarios for scenario_name
so that if prob is None you log an error with available keys (use logger.error
and list(stoch_config.scenarios.keys())) and raise a ValueError indicating the
missing scenario instead of assigning 1.0, leaving assignment to
instance._mpisppy_probability only after a valid prob is obtained.
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 93-94: The TODO about integrating TableWriter in
temoa/extensions/stochastics/stochastic_sequencer.py should be tracked as a
formal issue: create a repo issue describing the work (update handle_results or
add a new handle_stochastic_results to persist stochastics results via
TableWriter), include acceptance criteria (schema, persistence behavior, tests),
and then replace the inline TODO with a short note referencing the new issue
number/URL and the target symbols (TableWriter, handle_results,
StochasticSequencer) so future PRs can link to the tracker.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/source/computational_implementation.rstdocs/source/stochastics.rsttemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytests/testing_configs/config_utopia_stochastic.toml
🧰 Additional context used
🧠 Learnings (1)
📚 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/stochastics/stochastic_sequencer.pytemoa/extensions/stochastics/scenario_creator.py
🪛 Ruff (0.14.10)
temoa/extensions/stochastics/stochastic_config.py
21-23: Avoid specifying long messages outside the exception class
(TRY003)
61-64: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/stochastic_sequencer.py
21-21: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
43-43: Avoid specifying long messages outside the exception class
(TRY003)
55-58: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/scenario_creator.py
19-19: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
28-28: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (windows-latest, 3.13)
- GitHub Check: setup and test (macos-latest, 3.12)
- GitHub Check: setup and test (windows-latest, 3.12)
- GitHub Check: setup and test (macos-latest, 3.13)
🔇 Additional comments (11)
docs/source/computational_implementation.rst (1)
392-392: LGTM!The documentation update correctly reflects that stochastics is now tested and implemented in v4, with proper Sphinx doc reference formatting consistent with other extensions.
temoa/extensions/stochastics/scenario_creator.py (3)
19-19: LGTM -**kwargs: Anyis appropriate here.The static analysis warning about
Anyin kwargs can be safely ignored. The**kwargspattern is required for compatibility with mpi-sppy's scenario creator interface, and the function immediately validates and extracts the required typed arguments on lines 27-31.
108-128: LGTM - First-stage variable identification is correct.The logic correctly identifies first-stage variables as all
v_new_capacityentries for the first period, appropriately errors if none are found, and properly attaches the root node with the period cost expression for mpi-sppy.
39-44: No changes needed.The assumption that the value column is always the last one is not an unvalidated assumption—it is the explicit design contract documented in the
LoadItemclass docstring: "The last column is assumed to be the value for aParam." This contract is consistently applied across all 50+LoadItemdefinitions incomponent_manifest.py, confirming thatcolumns[:-1]correctly extracts the index columns by removing the final value column.Likely an incorrect or invalid review comment.
tests/testing_configs/config_utopia_stochastic.toml (1)
1-12: LGTM - Test configuration is well-structured.The configuration correctly sets up a stochastic scenario with appropriate settings and references the stochastic configuration file using a relative path.
temoa/extensions/stochastics/stochastic_config.py (2)
55-77: Perturbation validation is thorough and well-implemented.The validation correctly:
- Ensures referenced scenarios exist before creating perturbations
- Provides clear error messages with context (index, scenario name, available options)
- Defaults the action field to 'set' as documented
- Catches missing required fields with informative messages
The Ruff TRY003 warnings about long messages can be safely ignored here, as the detailed error messages significantly improve user experience when configuration errors occur.
78-88: LGTM - Final validation ensures configuration validity.The code appropriately validates that at least one scenario is defined and returns a properly constructed
StochasticConfiginstance with all parsed data.temoa/extensions/stochastics/stochastic_sequencer.py (4)
17-33: LGTM: Well-structured initialization with proper validation.The path validation and error handling are solid. The exception chaining with detailed logging will help with debugging configuration issues.
39-45: LGTM: Appropriate lazy import for optional dependency.The lazy import of mpi-sppy with clear error messaging is the right approach for an optional dependency.
78-91: LGTM: Robust error handling for optimization termination.The termination check using
pyo.check_optimal_termination()with defensive attribute access (lines 83-84) is well-implemented and will provide clear diagnostics on solve failures. The subsequent call toef.get_objective_value()correctly retrieves the extensive form objective value from mpi-sppy.
47-76: The code correctly follows the mpi-sppy API pattern:
ExtensiveFormcorrectly receives only the base options dict with the'solver'key (lines 50-52)- Solver-specific options are correctly passed exclusively to
solve_extensive_form(solver_options=...)(line 78)This separation aligns with mpi-sppy's API design, where
ExtensiveForm.__init__expects minimal options andsolve_extensive_form()accepts solver-specific configurations. No changes required.
ParticularlyPythonicBS
force-pushed
the
feat/stochastics
branch
from
53b89e6 to
03f34d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @temoa/core/config.py:
- Around line 233-246: The code silently ignores a present 'stochastic' section
when its 'stochastic_config' key is missing, which can cause confusing runtime
errors later in StochasticSequencer; update the block handling 'stochastic' (the
stoch_data / stochastic_config logic) to explicitly validate presence of
'stochastic_config' and either raise a clear ValueError (e.g.,
"stochastic_config missing in [stochastic] section") or log a warning and set
data['stochastic_config']=None so downstream code can handle it; ensure when
present you still resolve the path relative to config_file.parent as currently
done.
In @temoa/extensions/stochastics/scenario_creator.py:
- Around line 95-106: The code currently defaults missing scenario probability
to 1.0 which can corrupt expected-value calculations; instead, when
stoch_config.scenarios.get(scenario_name) returns None, raise a clear exception
(e.g., ValueError or KeyError) mentioning the missing scenario_name and that
probabilities are missing/invalid, and do not set instance._mpisppy_probability
in that case; update the block around stoch_config.scenarios, scenario_name,
logger, and instance._mpisppy_probability to perform this validation and raise
the error rather than defaulting.
- Around line 34-47: The catch-all Exception handler around the sqlite3.connect
block gives a misleading "Failed to connect to database" message even when
errors come from HybridLoader.create_data_dict or manifest processing; update
the try/except to either narrow the try to only the connection call or change
the exception handling to log the real error context: move the
sqlite3.connect(...) into its own small try/except (or use a separate try around
HybridLoader(...) and create_data_dict(...)), and in the except log both
temoa_config.input_database and the caught exception via logger.exception while
raising a RuntimeError that includes the original exception message (or a more
generic message that reflects which operation failed), referencing HybridLoader,
create_data_dict, manifest, table_index_map and temoa_config.input_database so
the log accurately reflects the failing step.
- Around line 72-90: The loop that applies perturbations over target_param
silently ignores unknown p.action values; update the code in the
scenario_creator loop that handles p.action (the if/elif chain handling
'multiply', 'add', 'set') to handle invalid actions explicitly by either raising
a clear ValueError (including p.action and any identifier like p.name or p) or
emitting a warning via the module logger with the same contextual info; add an
else branch after the existing elifs that performs this validation and stops or
warns so misconfigured perturbations are not silently dropped.
- Around line 19-28: Replace the untyped kwargs in scenario_creator with a
TypedDict describing the required keys: define a TypedDict (e.g.,
ScenarioKwargs) that requires 'temoa_config' and 'stoch_config' (with
appropriate types), import TypedDict from typing, then change the function
signature to accept kwargs: ScenarioKwargs (or a named param like params:
ScenarioKwargs) and update the runtime check to use those typed keys; ensure any
callers are updated to pass the typed mapping/argument and adjust imports
accordingly.
In @temoa/extensions/stochastics/stochastic_config.py:
- Around line 40-44: The code silently defaults dict-style scenarios to
probability 1.0; change the loop over scenarios_raw so that when isinstance(val,
dict) you first check if 'probability' in val and if not emit a warning (e.g.,
warnings.warn or module logger.warning) mentioning the scenario name, then parse
scenarios[name] = float(val.get('probability', 1.0)); update the code
referencing scenarios_raw, scenarios and val.get('probability', 1.0) accordingly
so missing probability keys are logged instead of being silently assumed.
In @temoa/extensions/stochastics/stochastic_sequencer.py:
- Around line 93-94: The TODO in StochasticSequencer (file
stochastic_sequencer.py) indicates result persistence is missing; create a
tracked GitHub issue and implement integration with the existing TableWriter by
either extending the StochasticSequencer.handle_results method or adding a new
method (e.g., persist_stochastic_results) that formats results and calls
TableWriter.write/insert, ensure error handling and idempotency, and add unit
tests exercising result persistence and a short docstring pointing to the new
issue number for follow-up.
- Around line 54-58: Remove the redundant validation that raises ValueError when
self.stoch_config.scenarios is empty in stochastic_sequencer.py; since
StochasticConfig.from_toml already enforces scenarios existence, delete the
if-block that checks self.stoch_config.scenarios and its raise, leaving
construction logic (e.g., ExtensiveForm-related setup) to rely on
StochasticConfig.from_toml's validation.
In @tests/test_full_runs.py:
- Around line 127-146: The test test_stochastic_utopia relies on a hard-coded
expected_obj (34389.9878) that assumes deterministic solver behavior; add a
concise inline comment above the expected_obj definition explaining how that
value was derived (e.g., derived from a baseline run with solver X version Y and
random seed Z or analytic expected value) and note that changes in
solver/version/seed may change the value, referencing expected_obj and
sequencer.stochastic_sequencer.objective_value so future maintainers know why
the constant exists and when to update it.
In @tests/testing_data/utopia_stochastic_config.toml:
- Around line 1-19: The TOML "column" field in perturbations is ignored: add a
column attribute to the Perturbation dataclass and parse it in the code that
reads TOML (the parsing logic in stochastic_config.py around the Perturbation
extraction), then update the perturbation application in scenario_creator.py to
check perturbation.column and only apply the change to matching column names
(or, if column-level behavior is not desired, remove the column entries from the
test file). Make sure to update any constructors/usages of Perturbation and the
unit tests to reflect the new column-aware behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
docs/source/computational_implementation.rstdocs/source/index.rstdocs/source/stochastics.rstpyproject.tomlrequirements-dev.txtrequirements.txttemoa/_internal/temoa_sequencer.pytemoa/core/config.pytemoa/core/modes.pytemoa/extensions/README.txttemoa/extensions/stochastics/EVPI.pytemoa/extensions/stochastics/README.txttemoa/extensions/stochastics/ReferenceModel.pytemoa/extensions/stochastics/VSS.pytemoa/extensions/stochastics/generate_scenario_tree-nonhomogenous.pytemoa/extensions/stochastics/generate_scenario_tree.pytemoa/extensions/stochastics/jobTemoa.pbstemoa/extensions/stochastics/legacy_files/ef_writer_script_old.pytemoa/extensions/stochastics/legacy_files/scenariomodels.pytemoa/extensions/stochastics/options/README.txttemoa/extensions/stochastics/options/__init__.pytemoa/extensions/stochastics/options/iew2012-nonhomogenized-markov.pytemoa/extensions/stochastics/options/iew2012.pytemoa/extensions/stochastics/options/utopia_coal_vs_nuc.pytemoa/extensions/stochastics/scenario_creator.pytemoa/extensions/stochastics/stochastic_config.pytemoa/extensions/stochastics/stochastic_sequencer.pytemoa/extensions/stochastics/stochastics_README.txttemoa/extensions/stochastics/temoa_stochastic.pytests/conftest.pytests/test_full_runs.pytests/testing_configs/config_utopia_stochastic.tomltests/testing_data/utopia_stochastic_config.toml
💤 Files with no reviewable changes (16)
- temoa/extensions/stochastics/stochastics_README.txt
- temoa/extensions/stochastics/options/iew2012.py
- temoa/extensions/stochastics/options/utopia_coal_vs_nuc.py
- temoa/extensions/stochastics/temoa_stochastic.py
- temoa/extensions/README.txt
- temoa/extensions/stochastics/VSS.py
- temoa/extensions/stochastics/options/README.txt
- temoa/extensions/stochastics/generate_scenario_tree.py
- temoa/extensions/stochastics/jobTemoa.pbs
- temoa/extensions/stochastics/options/iew2012-nonhomogenized-markov.py
- temoa/extensions/stochastics/legacy_files/ef_writer_script_old.py
- temoa/extensions/stochastics/ReferenceModel.py
- temoa/extensions/stochastics/legacy_files/scenariomodels.py
- temoa/extensions/stochastics/generate_scenario_tree-nonhomogenous.py
- temoa/extensions/stochastics/EVPI.py
- temoa/extensions/stochastics/README.txt
🧰 Additional context used
🧠 Learnings (1)
📚 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/stochastics/stochastic_sequencer.pytemoa/_internal/temoa_sequencer.pytemoa/extensions/stochastics/scenario_creator.py
🧬 Code graph analysis (4)
temoa/extensions/stochastics/stochastic_sequencer.py (3)
temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (2)
StochasticConfig(27-88)from_toml(34-88)temoa/extensions/stochastics/scenario_creator.py (1)
scenario_creator(19-130)
temoa/_internal/temoa_sequencer.py (2)
temoa/extensions/stochastics/stochastic_sequencer.py (1)
StochasticSequencer(12-94)temoa/core/modes.py (1)
TemoaMode(9-20)
tests/test_full_runs.py (2)
tests/conftest.py (1)
system_test_run(191-218)temoa/_internal/temoa_sequencer.py (1)
TemoaSequencer(45-272)
temoa/extensions/stochastics/scenario_creator.py (5)
temoa/_internal/run_actions.py (1)
build_instance(121-155)temoa/components/costs.py (1)
period_cost_rule(316-516)temoa/data_io/hybrid_loader.py (3)
HybridLoader(70-930)create_data_dict(161-281)data_portal_from_data(145-155)temoa/core/config.py (1)
TemoaConfig(32-344)temoa/extensions/stochastics/stochastic_config.py (1)
StochasticConfig(27-88)
🪛 Ruff (0.14.10)
temoa/extensions/stochastics/stochastic_config.py
21-23: Avoid specifying long messages outside the exception class
(TRY003)
61-64: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/stochastic_sequencer.py
21-21: Avoid specifying long messages outside the exception class
(TRY003)
25-25: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
43-43: Avoid specifying long messages outside the exception class
(TRY003)
55-58: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
temoa/extensions/stochastics/scenario_creator.py
19-19: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
28-28: Avoid specifying long messages outside the exception class
(TRY003)
47-47: Avoid specifying long messages outside the exception class
(TRY003)
121-121: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (windows-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 (macos-latest, 3.13)
🔇 Additional comments (20)
temoa/core/modes.py (1)
20-20: LGTM! Clean enum addition.The STOCHASTIC mode follows the existing sequential numbering pattern and integrates cleanly with the existing modes.
requirements.txt (1)
40-41: LGTM! Dependency tracking is consistent.The mpi-sppy dependency addition and comment updates properly reflect the dependency chain. The exact version pinning (==0.12.1) in this lock file is appropriate and consistent with the minimum version constraint (>=0.12.1) specified in pyproject.toml.
Also applies to: 52-52, 83-85, 105-105
docs/source/computational_implementation.rst (1)
392-392: LGTM! Documentation properly reflects the new stochastic capabilities.The update removes the "untested" warning and adds a proper cross-reference to the stochastics documentation, correctly reflecting that stochastic programming is now a supported feature.
requirements-dev.txt (1)
74-75: LGTM! Dev dependency tracking is consistent.The mpi-sppy dependency additions mirror the runtime requirements and properly track the dependency chain through commented references. The version pinning is consistent with requirements.txt.
Also applies to: 88-88, 127-129, 157-157
pyproject.toml (1)
33-33: mpi-sppy version 0.12.1 is valid and available on PyPI.Confirmed that version 0.12.1 exists and is part of an active release sequence. The latest version is 0.13.1, and no security advisories were detected. The dependency constraint
>=0.12.1appropriately allows version upgrades.temoa/extensions/stochastics/scenario_creator.py (2)
1-16: LGTM on imports and module setup.The imports are well-organized with proper TYPE_CHECKING guards for type hints that are only needed at static analysis time.
108-128: First-stage variable selection logic looks correct.The implementation correctly identifies first-period capacity decisions as first-stage variables and computes the first-stage cost using
period_cost_rule. The validation for empty first-stage variables is appropriate since stochastic optimization requires at least one first-stage decision.docs/source/index.rst (1)
9-9: LGTM - Documentation navigation updated for stochastics.The new stochastics page is appropriately added to the table of contents.
tests/conftest.py (1)
95-100: Clarify: The stochastic test usesutopia.sqliteas input, notutopia_stochastic.sqlite.Looking at
config_utopia_stochastic.toml, theinput_databaseistests/testing_outputs/utopia.sqlitewhileutopia_stochastic.sqliteis the output database. The(None, 'utopia_stochastic.sqlite')entry creates a schema-only database for output.This is correct but consider adding a comment for clarity:
📝 Optional: Add clarifying comment
# Feature tests (separate for temporal consistency) ('emissions.sql', 'emissions.sqlite'), ('materials.sql', 'materials.sqlite'), ('simple_linked_tech.sql', 'simple_linked_tech.sqlite'), ('storageville.sql', 'storageville.sqlite'), + # Output-only databases (schema only, no data) (None, 'utopia_stochastic.sqlite'),tests/testing_configs/config_utopia_stochastic.toml (1)
1-12: LGTM - Stochastic test configuration is well-structured.The configuration correctly sets up the stochastic mode and references the appropriate input/output databases and stochastic config file.
temoa/_internal/temoa_sequencer.py (2)
36-36: LGTM - Import and attribute for StochasticSequencer.The import and attribute declaration follow the established pattern for other sequencer types (MorrisSequencer, MgaSequencer, etc.).
Also applies to: 71-71
194-197: LGTM - STOCHASTIC mode integration follows established patterns.The implementation correctly:
- Creates
StochasticSequencerwith the config- Stores reference in
self.stochastic_sequencerfor later access (e.g., retrievingobjective_value)- Calls
start()to execute the stochastic runBased on retrieved learnings, the mode_override synchronization (lines 63-66) ensures
config.scenario_modeis correctly set beforeStochasticSequenceris instantiated, which is important for downstream consistency checks.tests/test_full_runs.py (1)
37-37: LGTM - Stochastic test files list defined.Follows the established pattern for
legacy_config_files,myopic_files, andmc_files.temoa/core/config.py (1)
57-58: LGTM! Stochastic config parameter integration is clean.The new
stochastic_configparameter follows the existing pattern of optional Path parameters and is correctly stored as an instance attribute.Also applies to: 148-148
docs/source/stochastics.rst (1)
1-107: LGTM! Comprehensive and well-structured documentation.The documentation clearly explains the stochastic extension, configuration requirements, perturbation options, and workflow. The limitations section is appropriately honest about current scope (two-stage only, partial result persistence).
temoa/extensions/stochastics/stochastic_config.py (2)
10-23: LGTM! Perturbation dataclass with proper validation.The
__post_init__validation for allowed actions is clean and provides a helpful error message.
55-76: LGTM! Robust perturbation parsing with good error handling.The validation that perturbations reference existing scenarios and the clear error messages for missing fields will help users debug configuration issues.
temoa/extensions/stochastics/stochastic_sequencer.py (3)
17-33: LGTM! Robust initialization with comprehensive path validation.The layered validation (missing config → path exists → is file → parse) provides clear error messages at each failure point.
80-91: LGTM! Proper termination checking and objective extraction.The code correctly validates optimal termination before accessing the objective value, with appropriate error logging and exception handling for non-optimal cases.
71-78: The code correctly follows the mpi-sppy API. Thesolver_optionsdict is properly passed tosolve_extensive_form()at the point where Pyomo solver configuration is applied, not at ExtensiveForm construction. No changes needed.Likely an incorrect or invalid review comment.
| 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)) | ||
|
|
||
| # Check if filter matches | ||
| match = True | ||
| for filter_key, filter_val in p.filter.items(): | ||
| if index_map.get(filter_key) != filter_val: | ||
| match = False | ||
| break | ||
|
|
||
| if match: | ||
| if p.action == 'multiply': | ||
| target_param[idx_tuple] = current_val * p.value | ||
| elif p.action == 'add': | ||
| target_param[idx_tuple] = current_val + p.value | ||
| elif p.action == 'set': | ||
| target_param[idx_tuple] = p.value | ||
|
|
There was a problem hiding this comment.
Missing validation for unknown perturbation actions.
If p.action is not one of multiply, add, or set, the perturbation is silently ignored. This could mask configuration errors. Consider logging a warning or raising an error for invalid actions.
🔧 Add validation for unknown actions
if match:
if p.action == 'multiply':
target_param[idx_tuple] = current_val * p.value
elif p.action == 'add':
target_param[idx_tuple] = current_val + p.value
elif p.action == 'set':
target_param[idx_tuple] = p.value
+ else:
+ logger.warning(
+ "Unknown perturbation action '%s' for scenario %s, table %s; skipping",
+ p.action,
+ scenario_name,
+ p.table,
+ )🤖 Prompt for AI Agents
In @temoa/extensions/stochastics/scenario_creator.py around lines 72 - 90, The
loop that applies perturbations over target_param silently ignores unknown
p.action values; update the code in the scenario_creator loop that handles
p.action (the if/elif chain handling 'multiply', 'add', 'set') to handle invalid
actions explicitly by either raising a clear ValueError (including p.action and
any identifier like p.name or p) or emitting a warning via the module logger
with the same contextual info; add an else branch after the existing elifs that
performs this validation and stops or warns so misconfigured perturbations are
not silently dropped.
| # 4. Attach root node (Stage 1) | ||
| periods = sorted(instance.time_optimize) | ||
| first_period = periods[0] | ||
|
|
||
| prob = stoch_config.scenarios.get(scenario_name) | ||
| if prob is None: | ||
| logger.warning( | ||
| "Scenario '%s' not found in stochastic config probabilities; defaulting to 1.0", | ||
| scenario_name, | ||
| ) | ||
| prob = 1.0 | ||
| instance._mpisppy_probability = prob |
There was a problem hiding this comment.
Defaulting missing scenario probability to 1.0 may cause incorrect expected value computation.
When a scenario is not found in stoch_config.scenarios, defaulting to probability 1.0 could lead to probabilities summing to more than 1.0 across all scenarios, producing incorrect expected value calculations in the stochastic optimization. This should likely raise an error instead of defaulting.
🔧 Recommended fix: Raise error for missing scenario
prob = stoch_config.scenarios.get(scenario_name)
if prob is None:
- logger.warning(
- "Scenario '%s' not found in stochastic config probabilities; defaulting to 1.0",
+ raise ValueError(
+ f"Scenario '{scenario_name}' not found in stochastic config probabilities. "
+ "All scenarios must have defined probabilities."
- scenario_name,
)
- prob = 1.0
instance._mpisppy_probability = prob📝 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.
| # 4. Attach root node (Stage 1) | |
| periods = sorted(instance.time_optimize) | |
| first_period = periods[0] | |
| prob = stoch_config.scenarios.get(scenario_name) | |
| if prob is None: | |
| logger.warning( | |
| "Scenario '%s' not found in stochastic config probabilities; defaulting to 1.0", | |
| scenario_name, | |
| ) | |
| prob = 1.0 | |
| instance._mpisppy_probability = prob | |
| # 4. Attach root node (Stage 1) | |
| periods = sorted(instance.time_optimize) | |
| first_period = periods[0] | |
| prob = stoch_config.scenarios.get(scenario_name) | |
| if prob is None: | |
| raise ValueError( | |
| f"Scenario '{scenario_name}' not found in stochastic config probabilities. " | |
| "All scenarios must have defined probabilities." | |
| ) | |
| instance._mpisppy_probability = prob |
🤖 Prompt for AI Agents
In @temoa/extensions/stochastics/scenario_creator.py around lines 95 - 106, The
code currently defaults missing scenario probability to 1.0 which can corrupt
expected-value calculations; instead, when
stoch_config.scenarios.get(scenario_name) returns None, raise a clear exception
(e.g., ValueError or KeyError) mentioning the missing scenario_name and that
probabilities are missing/invalid, and do not set instance._mpisppy_probability
in that case; update the block around stoch_config.scenarios, scenario_name,
logger, and instance._mpisppy_probability to perform this validation and raise
the error rather than defaulting.
| [scenarios] | ||
| low_coal = { probability = 0.5 } | ||
| high_coal = { probability = 0.5 } | ||
|
|
||
| [[perturbations]] | ||
| scenario = "low_coal" | ||
| table = "cost_variable" | ||
| column = "cost" | ||
| filter = { tech = "IMPHCO1" } | ||
| action = "multiply" | ||
| value = 0.5 | ||
|
|
||
| [[perturbations]] | ||
| scenario = "high_coal" | ||
| table = "cost_variable" | ||
| column = "cost" | ||
| filter = { tech = "IMPHCO1" } | ||
| action = "multiply" | ||
| value = 1.5 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for .column usage in the stochastics extension
rg -n "\.column" temoa/extensions/stochastics/Repository: TemoaProject/temoa
Length of output: 304
🏁 Script executed:
#!/bin/bash
# Also search for "column" in the file to catch broader patterns
rg -n "column" temoa/extensions/stochastics/ -iRepository: TemoaProject/temoa
Length of output: 575
🏁 Script executed:
#!/bin/bash
# Examine scenario_creator.py to verify the perturbation logic
find . -name "scenario_creator.py" -type fRepository: TemoaProject/temoa
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Search for perturbation-related code that might use column
rg -n "perturbations\|perturbation" temoa/extensions/stochastics/ -A 3 -B 1Repository: TemoaProject/temoa
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Read scenario_creator.py to see how perturbations are processed
cat -n temoa/extensions/stochastics/scenario_creator.py | head -100Repository: TemoaProject/temoa
Length of output: 4542
🏁 Script executed:
#!/bin/bash
# Search for Perturbation class or data structure definition
rg -n "class Perturbation|@dataclass.*Perturbation" temoa/extensions/stochastics/ -A 15Repository: TemoaProject/temoa
Length of output: 1342
🏁 Script executed:
#!/bin/bash
# Look for where TOML perturbations are loaded/parsed
rg -n "perturbations" temoa/extensions/stochastics/ -B 2 -A 5Repository: TemoaProject/temoa
Length of output: 3636
🏁 Script executed:
#!/bin/bash
# Read the complete Perturbation parsing section from TOML
sed -n '55,75p' temoa/extensions/stochastics/stochastic_config.pyRepository: TemoaProject/temoa
Length of output: 929
🏁 Script executed:
#!/bin/bash
# Check if column is mentioned anywhere in stochastic_config parsing or processing
rg -n "column" temoa/extensions/stochastics/stochastic_config.pyRepository: TemoaProject/temoa
Length of output: 44
Remove the unused column field from perturbations or implement column-specific filtering.
The column field (lines 8, 16) in the TOML perturbations is silently discarded: it is not defined in the Perturbation dataclass and is never extracted during TOML parsing in stochastic_config.py (lines 66-72). The perturbation logic in scenario_creator.py applies modifications to all matching index tuples regardless of column name. Either remove this field from the test config or implement proper column filtering if column-specific perturbations are intended.
🤖 Prompt for AI Agents
In @tests/testing_data/utopia_stochastic_config.toml around lines 1 - 19, The
TOML "column" field in perturbations is ignored: add a column attribute to the
Perturbation dataclass and parse it in the code that reads TOML (the parsing
logic in stochastic_config.py around the Perturbation extraction), then update
the perturbation application in scenario_creator.py to check perturbation.column
and only apply the change to matching column names (or, if column-level behavior
is not desired, remove the column entries from the test file). Make sure to
update any constructors/usages of Perturbation and the unit tests to reflect the
new column-aware behavior.
Revamped stochastics extension for Temoa v4 using mpi-sppy, adds mpi-sppy as a core dependency
Core Integration
Stochastic Components
Summary by CodeRabbit
New Features
Dependencies
Documentation
Removals
Tests
✏️ Tip: You can customize this high-level summary in your review settings.