Open
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR introduces evolving iteration functionality to the myopic optimization framework. It adds configuration parameters and execution hooks that allow custom evolution scripts to update the database during myopic iterations, alongside modifications to progress reporting and iteration sequencing logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Seq as MyopicSequencer
participant EvUp as EvolutionUpdater
participant DB as SQLite Database
loop Each Myopic Iteration
Seq->>Seq: Log iteration index
alt evolving == true AND not first iteration
Seq->>EvUp: run_evolution_script(idx, prev_base_year, status)
EvUp->>DB: Update via iterate() hook
DB-->>EvUp: Changes committed
EvUp-->>Seq: Evolution complete
Seq->>Seq: Report 'evolve' status
end
Seq->>Seq: Load demand data
Seq->>Seq: Construct TEMOA instance
Seq->>DB: Solve optimization
Seq->>Seq: Report 'solve' status
Seq->>Seq: Check results
Seq->>Seq: Report 'report' status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/extensions/myopic/myopic_sequencer.py`:
- Around line 213-221: Check that the path in self.evolution_script exists and
is a readable file (use os.path.exists/isfile) before calling
util.spec_from_file_location; after creating spec verify spec is not None and
spec.loader is not None before calling spec.loader.exec_module; once the module
is loaded verify hasattr(evolution_module, "iterate") and
callable(evolution_module.iterate) and only then call
evolution_module.iterate(...)—otherwise log/raise a clear, descriptive error
mentioning the missing file, spec, loader, or non-callable iterate so failures
are explicit and debuggable.
In `@temoa/tutorial_assets/config_sample.toml`:
- Line 157: The sample config uses a Windows-style path for the evolution_script
setting; update the evolution_script value (the "evolution_script" key) to use a
cross-platform path (POSIX-style forward slashes) so it resolves on Linux/macOS
and is safe to copy/paste (e.g., change the value from
"temoa\\extensions\\myopic\\evolution_updater.py" to a forward-slash form like
"temoa/extensions/myopic/evolution_updater.py").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18339bab-ae1c-43c9-ad62-14122d735c76
📒 Files selected for processing (8)
temoa/core/config.pytemoa/data_io/hybrid_loader.pytemoa/extensions/myopic/evolution_updater.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/tutorial_assets/config_sample.tomltests/test_myopic_sequencer.pytests/testing_configs/config_utopia_myopic.toml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
temoa/tutorial_assets/config_sample.toml (1)
156-156:⚠️ Potential issue | 🟠 MajorSet sample
evolvingdefault to non-evolving.
evolving = truein the sample template enables the compute-heavier path by default for copy/paste users, which contradicts the PR goal of defaulting to normal (non-evolving) myopic behavior.🔧 Proposed fix
-evolving = true +evolving = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/tutorial_assets/config_sample.toml` at line 156, The sample config currently sets evolving = true which makes the heavier evolving path the default; update the sample template to set the flag to non-evolving by changing the setting evolving = true to evolving = false (update the config key "evolving" in the sample file/template so copy/paste users get the normal myopic behavior by default).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/extensions/myopic/evolution_updater.py`:
- Around line 8-12: The code allows idx: MyopicIndex | None but later uses
idx.base_year unguarded; add an explicit check in the function (the constructor
or initializer that accepts idx) to validate idx is not None before accessing
idx.base_year (or handle the None case deterministically), e.g., if idx is None
raise a clear ValueError or provide a documented fallback; update any logic that
reads idx.base_year to run only after this guard so failures are explicit and
deterministic (refer to the parameter name idx and the type MyopicIndex and the
property base_year when locating the usage).
---
Duplicate comments:
In `@temoa/tutorial_assets/config_sample.toml`:
- Line 156: The sample config currently sets evolving = true which makes the
heavier evolving path the default; update the sample template to set the flag to
non-evolving by changing the setting evolving = true to evolving = false (update
the config key "evolving" in the sample file/template so copy/paste users get
the normal myopic behavior by default).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96b3e5d7-d97a-4301-b805-bacc6ad8bbbd
📒 Files selected for processing (3)
temoa/extensions/myopic/evolution_updater.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/tutorial_assets/config_sample.toml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/tutorial_assets/config_sample.toml`:
- Around line 149-155: Clarify that step_size is only meaningful when evolving
mode is enabled by updating the comment near the step_size setting: mention that
step_size controls how many periods the planner advances on each myopic
iteration when evolving mode (and evolution_script) is enabled, and that it must
be <= view_depth; also add a short note that when evolving mode is false,
step_size has no effect. Reference the existing symbols: step_size, evolving
mode (e.g., evolution_script), and view depth in the updated comment so users
understand the relationship.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d335026-e71d-4fbb-bf8d-1b8ef1ed4823
📒 Files selected for processing (2)
temoa/tutorial_assets/config_sample.tomltests/testing_configs/config_utopia_myopic.toml
ParticularlyPythonicBS
force-pushed
the
feature/myopic_evolving
branch
from
1133ff4 to
07c93c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
temoa/extensions/myopic/evolution_updater.py (1)
32-36: 🧹 Nitpick | 🔵 TrivialUse lazy logging format instead of f-string.
The f-string is evaluated eagerly even if the log level is disabled. Use
%-style formatting for deferred evaluation.♻️ Suggested cleanup
- logger.info(f"Running myopic iteration updater for base year {idx.base_year}") - - # Update your myopic database here. - - return + logger.info('Running myopic iteration updater for base year %s', idx.base_year) + + # Update your myopic database here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/myopic/evolution_updater.py` around lines 32 - 36, The log call in evolution_updater.py uses an eager f-string (logger.info(f"Running myopic iteration updater for base year {idx.base_year}")), which should be replaced with lazy %-style logging to avoid unnecessary formatting when the log level is disabled; update the call to use logger.info("Running myopic iteration updater for base year %s", idx.base_year) (referencing the existing logger and idx variables in the same scope).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@temoa/extensions/myopic/myopic_sequencer.py`:
- Around line 360-366: Change the type annotation of the run_evolution_script
parameter idx from int | None to MyopicIndex | None and update any related type
hints/usages accordingly (e.g., the progress_mapper.report call) so it accepts a
MyopicIndex; ensure MyopicIndex is imported or available in the module and
adjust function signature in def run_evolution_script(self, idx: MyopicIndex |
None, ...) and any callers that rely on that annotation to match the actual
MyopicIndex object passed from the main loop.
---
Duplicate comments:
In `@temoa/extensions/myopic/evolution_updater.py`:
- Around line 32-36: The log call in evolution_updater.py uses an eager f-string
(logger.info(f"Running myopic iteration updater for base year
{idx.base_year}")), which should be replaced with lazy %-style logging to avoid
unnecessary formatting when the log level is disabled; update the call to use
logger.info("Running myopic iteration updater for base year %s", idx.base_year)
(referencing the existing logger and idx variables in the same scope).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e846fbf-fa5e-431e-a80e-986f27c1a72f
📒 Files selected for processing (7)
temoa/core/config.pytemoa/extensions/myopic/evolution_updater.pytemoa/extensions/myopic/myopic_progress_mapper.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/tutorial_assets/config_sample.tomltests/test_myopic_sequencer.pytests/testing_configs/config_utopia_myopic.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Creates two versions of myopic mode: normal and evolving.
Evolving myopic:
We call an "evolution script" between iterations, which may be used to update the myopic database (e.g., to "surprise" the optimisation as it moves forward, or to update parameters based on past decisions). In this mode, the problem may change going forward and so we need to "shrink" the view depth as we approach the end of the horizon, so that we make decisions at every defined step.
Normal (non-evolving) myopic:
We do not call the evolution script. As a result, the period which views to the end of the horizon should be the final planning period. There is no need to shrink the view depth and step forward as the model should make identical decisions, as this is just a subproblem of the previous problem.
NOTE: Evolving myopic (with no evolution script) is currently the default in Temoa, but this would make normal (non-evolving) the default behaviour as it saves unnecessary compute.
Utopia testing for all possible view depths and step sizes:
Old behaviour:
log_baseline.txt
New normal (non-evolving) behaviour for all possible view depths and step sizes:
log_new_non-evolving.txt
New evolving behaviour for all possible view depths and step sizes:
log_new_evolving.txt
Summary by CodeRabbit
New Features
Improvements
Configuration
evolving(bool) andevolution_script(string).Tests