Feature/308 rename effect domains #360
Conversation
WalkthroughRenamed Effect domains and parameters from operation/invest to temporal/nontemporal across core models, features, elements, results, examples, and tests. Introduced per_timestep naming, updated result keys (e.g., costs|total → costs), added kwargs validation, and provided backward-compatible deprecated properties and warnings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Eff as Effect (API)
participant EM as EffectModel
participant SAM as ShareAllocationModel
participant Sol as Solver
participant Res as CalculationResults
Dev->>Eff: Create Effect(minimum_temporal, maximum_nontemporal, ...)\n(or deprecated names via **kwargs)
note over Eff: Deprecated names mapped with warnings
Eff->>EM: Build model components\nnontemporal, temporal, per_timestep
EM->>SAM: Add shares targeting\n[temporal | nontemporal]
EM->>Sol: Define variables/constraints\n- Effect\n- Effect(temporal)|per_timestep\n- Effect(nontemporal)
Sol-->>EM: Solutions for variables
EM-->>Res: Populate results keys\n- costs\n- costs(temporal)|per_timestep\n- costs(nontemporal)
Dev->>Res: Access results by new keys
note over Res: Old keys not used in tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)flixopt/effects.py (3)
⏰ 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)
🔇 Additional comments (12)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/effects.py (1)
567-589: Circular-share check can silently fail with string keys (labels)create_effect_values_dict promotes keys to labels; then comparing Effect objects against string keys will never match, weakening loop detection.
- for effect in self.effects.values(): - # Effekt darf nicht selber als Share in seinen ShareEffekten auftauchen: - # operation: - for target_effect in effect.specific_share_to_other_effects_operation.keys(): - assert effect not in self[target_effect].specific_share_to_other_effects_operation.keys(), ( - f'Error: circular operation-shares \n{error_str(effect.label, self[target_effect].label)}' - ) - # invest: - for target_effect in effect.specific_share_to_other_effects_invest.keys(): - assert effect not in self[target_effect].specific_share_to_other_effects_invest.keys(), ( - f'Error: circular invest-shares \n{error_str(effect.label, self[target_effect].label)}' - ) + def key_label(k): # supports both Effect and str keys + return k.label if isinstance(k, Effect) else k + + for effect in self.effects.values(): + # temporal (formerly operation) + for target_label in effect.specific_share_to_other_effects_operation.keys(): + peer = self[target_label] + peer_keys = {key_label(k) for k in peer.specific_share_to_other_effects_operation.keys()} + assert effect.label not in peer_keys, ( + f'Error: circular temporal-shares \n{error_str(effect.label, peer.label)}' + ) + # nontemporal (formerly invest) + for target_label in effect.specific_share_to_other_effects_invest.keys(): + peer = self[target_label] + peer_keys = {key_label(k) for k in peer.specific_share_to_other_effects_invest.keys()} + assert effect.label not in peer_keys, ( + f'Error: circular nontemporal-shares \n{error_str(effect.label, peer.label)}' + )
🧹 Nitpick comments (6)
flixopt/structure.py (1)
225-233: Generalize var-keyword detection in_validate_kwargs.Right now we special-case the literal name
"kwargs"when buildingknown_params. Any__init__that uses a different var-keyword name (e.g.,**options) would still allow arbitrary keywords, but this helper would flag them as unexpected. That makes_validate_kwargsbrittle for future classes that adopt the helper with differently named**parameters. Please deriveknown_paramsusinginspect.Parameter.kind != VAR_KEYWORDso all var-keyword names are excluded automatically.- sig = inspect.signature(self.__init__) - known_params = set(sig.parameters.keys()) - {'self', 'kwargs'} + sig = inspect.signature(self.__init__) + known_params = { + name + for name, param in sig.parameters.items() + if name != 'self' and param.kind != inspect.Parameter.VAR_KEYWORD + }flixopt/results.py (1)
7-7: Remove unused import.
warningsis imported but not used in this file.flixopt/features.py (1)
956-965: Variable/constraint name collision (same label) is intentional—please confirm uniqueness assumptions.
self.totaland its constraint both namedself.label_full. Linopy keeps variables and constraints separate, but double-check any consumers that treat names as unique across both sets.If needed, consider a distinct constraint short name (e.g., '|total_eq') to ease debugging while preserving public variable naming.
tests/test_effect.py (1)
114-116: Legacy share parameter names: confirm deprecation path.
specific_share_to_other_effects_operation/_investstill appear. If renamed to temporal/nontemporal, update tests; otherwise ensure deprecation support is active.Would you like me to update tests to the new names (e.g.,
specific_share_to_other_effects_temporal/_nontemporal) if available?flixopt/effects.py (2)
153-161: Deprecation compatibility layer is solid; minor cleanup suggestedLogic correctly maps old kwargs to new ones and prevents double-specification. Two small improvements:
- Drop repeated local imports of warnings and rely on the module-level import.
- Optionally use class_name in _validate_kwargs for clearer error messages.
- import warnings + # use module-level warnings import - import warnings + # use module-level warnings importIf desired:
- self._validate_kwargs(kwargs) + self._validate_kwargs(kwargs, class_name=self.__class__.__name__)Also applies to: 174-265
414-420: Align TimeSeries labels with 'temporal' terminologyLabels still use 'operation' in prefixes/suffixes; update to 'temporal' for consistency. This affects metadata/IO names, not model math.
- self.specific_share_to_other_effects_operation = flow_system.create_effect_time_series( - f'{self.label_full}|operation->', self.specific_share_to_other_effects_operation, 'operation' - ) + self.specific_share_to_other_effects_operation = flow_system.create_effect_time_series( + f'{self.label_full}|temporal->', self.specific_share_to_other_effects_operation, 'temporal' + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
CHANGELOG.md(1 hunks)examples/03_Calculation_types/example_calculation_types.py(1 hunks)flixopt/calculation.py(1 hunks)flixopt/components.py(5 hunks)flixopt/effects.py(8 hunks)flixopt/elements.py(1 hunks)flixopt/features.py(7 hunks)flixopt/results.py(5 hunks)flixopt/structure.py(1 hunks)tests/conftest.py(1 hunks)tests/test_effect.py(3 hunks)tests/test_flow.py(5 hunks)tests/test_functional.py(2 hunks)tests/test_integration.py(4 hunks)tests/test_io.py(1 hunks)tests/test_linear_converter.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
tests/test_linear_converter.py (3)
flixopt/results.py (4)
constraints(235-239)constraints(385-393)variables(228-232)variables(374-382)flixopt/structure.py (2)
constraints(462-463)variables(458-459)tests/conftest.py (1)
assert_conequal(455-471)
flixopt/components.py (1)
flixopt/structure.py (1)
_validate_kwargs(205-233)
tests/test_flow.py (3)
flixopt/results.py (4)
constraints(235-239)constraints(385-393)variables(228-232)variables(374-382)flixopt/structure.py (2)
constraints(462-463)variables(458-459)tests/conftest.py (1)
assert_conequal(455-471)
tests/test_effect.py (2)
flixopt/structure.py (4)
constraints(462-463)variables(458-459)coords(105-106)hours_per_step(97-98)tests/conftest.py (3)
assert_var_equal(474-508)assert_conequal(455-471)create_linopy_model(424-427)
flixopt/results.py (2)
flixopt/structure.py (2)
label(415-416)solution(76-94)flixopt/io.py (1)
save_dataset_to_netcdf(207-243)
flixopt/features.py (3)
flixopt/effects.py (2)
effects(625-626)add_share_to_effects(663-675)flixopt/structure.py (4)
coords(105-106)label_full(312-313)label_full(419-425)add(365-389)flixopt/elements.py (1)
label_full(481-482)
flixopt/effects.py (3)
flixopt/structure.py (4)
_validate_kwargs(205-233)label_full(312-313)label_full(419-425)add(365-389)flixopt/flow_system.py (1)
create_time_series(349-373)flixopt/features.py (2)
ShareAllocationModel(923-1024)add_share(989-1024)
examples/03_Calculation_types/example_calculation_types.py (2)
flixopt/core.py (1)
to_dataframe(738-767)flixopt/plotting.py (1)
with_plotly(328-463)
flixopt/calculation.py (1)
flixopt/structure.py (1)
solution(76-94)
tests/test_io.py (1)
flixopt/structure.py (1)
solution(76-94)
tests/test_functional.py (1)
flixopt/results.py (2)
variables(228-232)variables(374-382)
tests/test_integration.py (2)
flixopt/structure.py (1)
solution(76-94)flixopt/results.py (1)
solution_without_overlap(865-878)
⏰ 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: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (22)
tests/test_flow.py (5)
102-115: Terminology rename looks correct for temporal effects on flow hours.Assertions align with ShareAllocationModel naming and dimensions.
405-419: Non‑temporal investment effects wiring is correct.Variables/constraints names and expression semantics match the new nontemporal domain.
440-446: Divest effect constraint matches intended formulation.Constraint enforces var == (1 - is_invested) * 500.
543-556: Temporal running-hour effects wiring is correct.Time-series constraint uses on * hours_per_step * factor as expected.
889-895: Startup cost mapped to temporal domain is correct.Constraint equals per-switch_on cost; naming is consistent.
flixopt/features.py (7)
89-90: Use of nontemporal for fix effects is correct.Matches the new domain naming.
100-101: Divest effects correctly target nontemporal.Semantics preserved.
107-108: Specific effects mapped to nontemporal look good.Consistent with investment sizing.
739-740: Running-hour effects correctly target temporal.Time-based contributions go to the temporal domain.
749-750: Per‑switch effects to temporal is appropriate.Discrete switch events are time-indexed.
967-984: per_timestep renaming and bounds are correct.Bounds correctly scale per-hour limits by
hours_per_step; naming matches tests.
1079-1080: Piecewise effect shares routed to nontemporal are correct.Aggregation logic consistent with non-temporal totals.
tests/test_effect.py (5)
18-34: Effect variable/constraint renames (temporal/nontemporal) look correct.Names and dimensions align with ShareAllocationModel changes.
57-65: Bounds rename coverage is correct.
minimum_temporal,maximum_temporal,minimum_nontemporal,maximum_nontemporal, and per-hour limits are used as intended.
83-106: Constraint wiring for totals and per_timestep is consistent.Totals sum temporal+nontemporal; temporal equals sum of per_timestep.
123-137: Cross‑effect variable naming is correct.
Effect1(nontemporal)->Effect2(nontemporal)and... (temporal)match new scheme.
139-160: Cross‑effect share constraints are correctly formulated.Nontemporal maps scalar‑to‑scalar; temporal maps per_timestep to per_timestep.
tests/test_integration.py (2)
66-70: Result key renames look consistent with the new temporal/nontemporal APIThe updated accessors (costs, CO2, costs(temporal)|per_timestep, CO2(temporal), CO2(nontemporal), and the component->effect paths) align with ShareAllocationModel naming and EffectModel totals. Good coverage across scalar totals and per_timestep arrays.
Also applies to: 182-201, 204-233, 235-255
410-419: No residual old ‘operation’ keys detected: scan found only CHANGELOG mappings and the intended deprecation handlers in effects.py.flixopt/effects.py (3)
55-73: Docstring rename is clear and complete for boundsParameters reflect temporal/nontemporal and per-hour terminology with deprecation notes. Looks good.
441-468: Model split into temporal/nontemporal is correctNaming (…(temporal), …(nontemporal), …|per_timestep) and bounds wiring (total, per-hour) are consistent with ShareAllocationModel and tests.
Also applies to: 480-488
697-709: Cross-effect share routing is correct under new domainsTemporal uses total_per_timestep * TS; nontemporal uses scalar total * factor. Naming yields CO2(temporal)->costs(temporal) as expected.
Renamed
Effectdomains:invest→nontemporaloperation→temporalFurther, rename
Effectparameters to:minimum_investment→minimum_nontemporalmaximum_investment→maximum_nontemporalminimum_operation→minimum_temporalmaximum_operation→maximum_temporalminimum_operation_per_hour→minimum_per_hourmaximum_operation_per_hour→maximum_per_hourType of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests