Add output threshold and myopic capacity config#272
Open
SutubraResearch wants to merge 4 commits intoTemoaProject:unstablefrom
Open
Add output threshold and myopic capacity config#272SutubraResearch wants to merge 4 commits intoTemoaProject:unstablefrom
SutubraResearch wants to merge 4 commits intoTemoaProject:unstablefrom
Conversation
WalkthroughThis PR adds per-type output filtering thresholds for capacity, activity, emission, and cost. TemoaConfig gains four new optional threshold fields. TableWriter resolves and applies per-type thresholds (with validation and defaults) throughout output generation and cost row writing. The myopic sequencer uses a configurable capacity threshold instead of a hard-coded epsilon. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temoa/_internal/table_writer.py (1)
84-111: 🧹 Nitpick | 🔵 TrivialThe
epsilonparameter is effectively dead code.The
__init__signature acceptsepsilon: float = 1e-5at Line 84, but this value is immediately overwritten at Lines 106-111 with the minimum of the resolved per-type thresholds. Any caller passing anepsilonvalue will have it silently ignored.Consider removing the parameter to avoid confusion:
♻️ Proposed fix
- def __init__(self, config: TemoaConfig, epsilon: float = 1e-5) -> None: + def __init__(self, config: TemoaConfig) -> None: self.config = config - self.epsilon = epsilon self.tech_sectors: dict[str, str] | None = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/_internal/table_writer.py` around lines 84 - 111, The __init__ parameter epsilon is unused because self.epsilon is overwritten later by the min of resolved thresholds (_resolve_output_threshold), so remove the epsilon parameter from the __init__ signature and any initial assignment (remove "epsilon: float = 1e-5" and the early "self.epsilon = epsilon"), leaving the final self.epsilon = min(...) logic intact; update any call sites / tests that instantiate this class to stop passing an epsilon and adjust any docstrings or type annotations referencing the constructor parameter.
🤖 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 67-75: Update the sample TOML to document the new myopic option:
add a capacity_threshold entry under the [myopic] section (matching the default
used by the code where config.myopic_inputs.get('capacity_threshold',
default_cap_threshold) sets self.capacity_epsilon) with the default value 10 and
a short comment like "minimum capacity (MW) to carry forward between periods" so
users can discover and override the capacity_threshold used by the myopic
sequencer.
---
Outside diff comments:
In `@temoa/_internal/table_writer.py`:
- Around line 84-111: The __init__ parameter epsilon is unused because
self.epsilon is overwritten later by the min of resolved thresholds
(_resolve_output_threshold), so remove the epsilon parameter from the __init__
signature and any initial assignment (remove "epsilon: float = 1e-5" and the
early "self.epsilon = epsilon"), leaving the final self.epsilon = min(...) logic
intact; update any call sites / tests that instantiate this class to stop
passing an epsilon and adjust any docstrings or type annotations referencing the
constructor parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2dd4959-b16b-4a93-bd60-6ec6438a7a36
📒 Files selected for processing (5)
temoa/_internal/table_writer.pytemoa/core/config.pytemoa/db_schema/temoa_schema_v4.sqltemoa/extensions/myopic/myopic_sequencer.pytemoa/tutorial_assets/config_sample.toml
SutubraResearch
force-pushed
the
pr/small-features
branch
from
39a0231 to
5dbc761
Compare
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 95-96: The comment about precedence is inaccurate because
table_writer.py::_resolve_output_threshold currently only reads the TOML config
value and falls back to OUTPUT_THRESHOLD_DEFAULTS without checking any
"metadata_real in output db"; either update the doc comment to reflect actual
behavior ("Precedence is: TOML value > internal defaults") or implement the
missing DB lookup by adding a metadata read in _resolve_output_threshold that
checks the output DB for metadata_real and uses it as the middle precedence
before falling back to OUTPUT_THRESHOLD_DEFAULTS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cf090f8-d51a-4cfd-a7ff-c07c5a732b6a
📒 Files selected for processing (11)
temoa/_internal/table_writer.pytemoa/core/config.pytemoa/extensions/myopic/myopic_sequencer.pytemoa/tutorial_assets/config_sample.tomltests/testing_data/mediumville_sets.jsontests/testing_data/test_system_sets.jsontests/testing_data/utopia_sets.jsontests/utilities/capture_set_values_for_cache.pytests/utilities/config_mediumville.tomltests/utilities/config_test_system.tomltests/utilities/config_utopia.toml
Add output_threshold config option to filter near-zero values from result tables. Defaults to 0 (no filtering). Controlled entirely via config file, no database schema changes.
SutubraResearch
force-pushed
the
pr/small-features
branch
from
5dbc761 to
4c7733f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
temoa/extensions/myopic/myopic_sequencer.py (1)
67-69:⚠️ Potential issue | 🟡 MinorPlease add
capacity_thresholdto the sample[myopic]config docs.This code advertises TOML configurability, but
temoa/tutorial_assets/config_sample.tomlstill does not show the option under[myopic], which makes discovery difficult for users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temoa/extensions/myopic/myopic_sequencer.py` around lines 67 - 69, The sample config is missing the advertised TOML key for the myopic capacity threshold; add a capacity_threshold entry under the [myopic] section in temoa/tutorial_assets/config_sample.toml (key name: capacity_threshold) with a brief comment and the default value 10 to match default_cap_threshold in myopic_sequencer.py so users can discover and override the setting.
🤖 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/_internal/table_writer.py`:
- Around line 249-260: The _validate_threshold function currently allows
non-finite floats; update it to reject NaN and infinities by checking
math.isfinite(numeric_value) after converting threshold_value to float and
raising ValueError if not finite (include threshold_name in the message); use
the standard library math.isfinite to perform the check in the
_validate_threshold method so non-finite values are rejected during config
parsing.
- Around line 102-111: The constructor currently ignores a passed-in epsilon
because after resolving per-type thresholds via _resolve_output_threshold() you
overwrite epsilon with the minimum of those resolved values; update the
constructor so that if an explicit epsilon argument is provided it is applied to
all per-type thresholds (output_threshold_capacity, output_threshold_activity,
output_threshold_emission, output_threshold_cost) before or instead of calling
_resolve_output_threshold(), or else mark epsilon deprecated and remove it;
specifically, detect a non-None/explicit epsilon value in __init__ and assign it
to each output_threshold_* or bypass _resolve_output_threshold() accordingly,
ensuring self.epsilon retains the caller-provided value and that
_resolve_output_threshold() is only used for unspecified per-type threshold
values.
In `@temoa/extensions/myopic/myopic_sequencer.py`:
- Around line 67-75: The carry-forward delete/query in
update_myopic_efficiency_table is missing the capacity threshold check so
[myopic].capacity_threshold (stored on self.capacity_epsilon) is ignored; update
the SQL/ORM delete and any selection logic in update_myopic_efficiency_table to
only carry forward rows whose capacity magnitude meets the configured threshold
(e.g., add a WHERE ABS(capacity) >= :capacity_epsilon or capacity >=
:capacity_epsilon using self.capacity_epsilon), and ensure the parameter is
passed as a numeric value from the initialized self.capacity_epsilon.
---
Duplicate comments:
In `@temoa/extensions/myopic/myopic_sequencer.py`:
- Around line 67-69: The sample config is missing the advertised TOML key for
the myopic capacity threshold; add a capacity_threshold entry under the [myopic]
section in temoa/tutorial_assets/config_sample.toml (key name:
capacity_threshold) with a brief comment and the default value 10 to match
default_cap_threshold in myopic_sequencer.py so users can discover and override
the setting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d533b442-237e-49ef-8f30-625d6a9a5fa5
📒 Files selected for processing (4)
temoa/_internal/table_writer.pytemoa/core/config.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/extensions/myopic/myopic_sequencer.py`:
- Around line 67-75: The value assigned to self.capacity_epsilon must be
validated like TableWriter._validate_threshold(): retrieve the raw value from
config.myopic_inputs.get('capacity_threshold', default_cap_threshold), ensure it
is finite (not NaN/inf) and >= 0, and either call/reuse
TableWriter._validate_threshold(raw_value) or perform the same checks and raise
ValueError (or fall back to default_cap_threshold) if invalid; update the
assignment of capacity_epsilon accordingly so negative or non-finite config
values cannot be used in later SQL comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4546d0ad-f9a7-428f-b8a9-cac862aa575e
📒 Files selected for processing (2)
temoa/_internal/table_writer.pytemoa/extensions/myopic/myopic_sequencer.py
Comment on lines
+67
to
+75
| # Minimum capacity (MW) to carry forward between myopic periods. | ||
| # Configurable via [myopic] capacity_threshold in TOML. | ||
| default_cap_threshold = 1e-3 | ||
| if config and config.myopic_inputs: | ||
| self.capacity_epsilon = config.myopic_inputs.get( | ||
| 'capacity_threshold', default_cap_threshold | ||
| ) | ||
| else: | ||
| self.capacity_epsilon = default_cap_threshold |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing validation for capacity_threshold from config.
TableWriter._validate_threshold() rejects negative and non-finite values, but here the value from config.myopic_inputs.get('capacity_threshold', ...) is used directly without validation. If a user configures a negative, NaN, or inf value, the SQL query behavior becomes undefined (e.g., ABS(capacity) >= NaN is always false).
Consider adding validation consistent with TableWriter:
♻️ Proposed fix to add validation
+import math
+
+def _validate_capacity_threshold(value: float, default: float) -> float:
+ """Validate capacity threshold, returning default if invalid."""
+ if not isinstance(value, (int, float)):
+ return default
+ if not math.isfinite(value) or value < 0:
+ return default
+ return float(value)
+
class MyopicSequencer:
...
def __init__(self, config: TemoaConfig | None):
# Minimum capacity (MW) to carry forward between myopic periods.
# Configurable via [myopic] capacity_threshold in TOML.
default_cap_threshold = 1e-3
if config and config.myopic_inputs:
- self.capacity_epsilon = config.myopic_inputs.get(
+ raw_threshold = config.myopic_inputs.get(
'capacity_threshold', default_cap_threshold
)
+ self.capacity_epsilon = _validate_capacity_threshold(raw_threshold, default_cap_threshold)
else:
self.capacity_epsilon = default_cap_threshold📝 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.
Suggested change
| # Minimum capacity (MW) to carry forward between myopic periods. | |
| # Configurable via [myopic] capacity_threshold in TOML. | |
| default_cap_threshold = 1e-3 | |
| if config and config.myopic_inputs: | |
| self.capacity_epsilon = config.myopic_inputs.get( | |
| 'capacity_threshold', default_cap_threshold | |
| ) | |
| else: | |
| self.capacity_epsilon = default_cap_threshold | |
| # Minimum capacity (MW) to carry forward between myopic periods. | |
| # Configurable via [myopic] capacity_threshold in TOML. | |
| default_cap_threshold = 1e-3 | |
| if config and config.myopic_inputs: | |
| raw_threshold = config.myopic_inputs.get( | |
| 'capacity_threshold', default_cap_threshold | |
| ) | |
| self.capacity_epsilon = _validate_capacity_threshold(raw_threshold, default_cap_threshold) | |
| else: | |
| self.capacity_epsilon = default_cap_threshold |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@temoa/extensions/myopic/myopic_sequencer.py` around lines 67 - 75, The value
assigned to self.capacity_epsilon must be validated like
TableWriter._validate_threshold(): retrieve the raw value from
config.myopic_inputs.get('capacity_threshold', default_cap_threshold), ensure it
is finite (not NaN/inf) and >= 0, and either call/reuse
TableWriter._validate_threshold(raw_value) or perform the same checks and raise
ValueError (or fall back to default_cap_threshold) if invalid; update the
assignment of capacity_epsilon accordingly so negative or non-finite config
values cannot be used in later SQL comparisons.
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
1 participant
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.
Summary
Two independent small features that add configurability:
output_thresholdparameters to config that filter near-zero flow values from output tables. Reduces output database size significantly for large models where many technology-vintage combinations have negligible flows. Default 0.01 preserves current behaviour for small models.Files changed
temoa/_internal/table_writer.pytemoa/core/config.pytemoa/db_schema/temoa_schema_v4.sqltemoa/tutorial_assets/config_sample.tomltemoa/extensions/myopic/myopic_sequencer.pyTest plan
Summary by CodeRabbit