Feature/v3/feature/308 rename effect domains

[FBumann]

Renamed Effect domains:

invest → nontemporal
operation→ temporal

Further, rename Effect parameters to:

• minimum_investment → minimum_nontemporal
• maximum_investment → maximum_nontemporal
• minimum_operation → minimum_temporal
• maximum_operation → maximum_temporal
• minimum_operation_per_hour → minimum_per_hour
• maximum_operation_per_hour → maximum_per_hour

Type of Change

• Bug fix
• New feature
• Documentation update
• Code refactoring

Related Issues

Closes #(issue number)

Testing

• I have tested my changes
• Existing tests still pass

Checklist

• My code follows the project style
• I have updated documentation if needed
• I have added tests for new functionality (if applicable)

Summary by CodeRabbit

• New Features
  • Added runtime validation to reject unexpected keyword arguments during object creation.
• Refactor
  • Renamed “operation/invest” to “temporal/nontemporal” across effects, results, and examples.
  • Updated public parameters and result identifiers (e.g., costs|total → costs; per-timestep keys aligned to temporal).
• Documentation
  • Documented deprecated Effect parameters and their new names.
• Tests
  • Updated test suite to reflect new terminology and result keys.
• Chores
  • Improved deprecation warnings and messaging consistency.
• Examples
  • Adjusted plotting/data references and comparison columns to use temporal/nontemporal.

[coderabbitai]

Walkthrough

Renames the Effect domains from operation/invest to temporal/nontemporal across core models, results, examples, and tests. Updates parameter names, labels, dataset keys, and plotting references. Adds deprecation handling and kwargs validation. Adjusts result aggregation paths and per-timestep naming. Documentation and tests updated accordingly.

Changes

Cohort / File(s)	Summary
Documentation CHANGELOG.md	Added deprecation note documenting Effect parameter renames (operation/invest → temporal/nontemporal and per-hour keys).
Examples examples/03_Calculation_types/example_calculation_types.py, examples/05_Two-stage-optimization/two_stage_optimization.py	Updated plotting/data keys and comparison columns: operation/invest → temporal/nontemporal; total_per_timestep → per_timestep; costs
Effects and Model Wiring flixopt/effects.py, flixopt/elements.py, flixopt/features.py, flixopt/calculation.py	Core refactor: rename submodels, targets, labels, parameters from operation/invest to temporal/nontemporal; total = temporal + nontemporal; per-timestep key renamed; share targets updated; added deprecated properties and kwargs mapping.
Results and Datasets flixopt/results.py	Public/internal mode literals updated to ['temporal','nontemporal','total']; dataset suffixes and aggregation adjusted; APIs accept/return new modes; total computation sums temporal+nontemporal.
Validation Utility and Components flixopt/structure.py, flixopt/components.py	Added Interface._validate_kwargs; invoked in Source/ Sink/ SourceAndSink to reject unexpected kwargs; refined deprecation messages.
Tests tests/test_* (…test_effect.py, test_flow.py, test_functional.py, test_integration.py, test_io.py, test_linear_converter.py, test_scenarios.py)	Updated assertions and keys to temporal/nontemporal naming; costs

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Interface as Interface.__init__
  participant Validator as _validate_kwargs
  participant Component as Source/Sink/SourceAndSink

  User->>Component: __init__(..., **kwargs)
  Component->>Validator: _validate_kwargs(kwargs, class_name)
  alt unexpected kwargs
    Validator-->>Component: TypeError
    Component-->>User: raise
  else ok
    Component-->>User: constructed
  end

Loading

sequenceDiagram
  autonumber
  participant Builder as EffectModel
  participant Temporal as Submodel(temporal)
  participant NonTemporal as Submodel(nontemporal)
  participant Results as CalculationResults

  Builder->>Temporal: build constraints (min/max, per_timestep)
  Builder->>NonTemporal: build constraints (min/max)
  Builder->>Builder: total = temporal.total + nontemporal.total
  Results->>Temporal: fetch temporal values
  Results->>NonTemporal: fetch nontemporal values
  Results->>Results: compute total/merge datasets with new keys

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

• [FEATURE] Rename Effect domains #308 — Implements the rename from operation/invest to temporal/nontemporal across Effects, models, results, and labels, matching this PR’s changes.

Possibly related PRs

• Feature/308 rename effect domains #360 — Parallel refactor of effect domains and keys to temporal/nontemporal across effects, results, examples, and tests.
• Feature/v3/data converter #356 — Modifies Effect/EffectModel interfaces with the same domain rename, overlapping code paths.
• V3.0.0/main #284 — Similar API and label renaming in effects/results/structure affecting the same modules.

Suggested reviewers

• baumbude
• PStange

Poem

Ears up, I hop through temporal time,
Nontemporal burrows—tidy, sublime.
Shares are mapped, constraints align,
Costs per hop now neatly rhyme.
Old names fade like dusk’s soft glow—
New labels sprout; onward we go! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The title includes an unnecessary branch/feature prefix and does not follow the concise, descriptive format expected; although it mentions “rename effect domains,” the extra “Feature/v3/feature/308” text adds noise and detracts from clarity.	Please simplify the title to a clear, single sentence such as “Rename effect domains from invest/operation to nontemporal/temporal” and remove any branch or ticket prefixes.
Description Check	⚠️ Warning	The description does not match the repository template because it uses a custom “## Renamed Effect domains” header instead of “## Description,” leaves the Related Issues placeholder without a valid issue number, and the Testing section checkboxes are all unchecked, making it unclear if tests were run.	Please update the description to use the “## Description” header, fill in the actual issue number under Related Issues, and mark or complete the Testing checkboxes to confirm that changes have been tested and existing tests still pass.
Docstring Coverage	⚠️ Warning	Docstring coverage is 59.32% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature/v3/feature/308-rename-effect-domains-final

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[FBumann]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

flixopt/effects.py (1)

391-426: Verify cross-effect share transformation.

Line 422 appears to have an inconsistency - it uses 'operation->' as the prefix but processes specific_share_to_other_effects_invest data. This seems like a copy-paste error that should be 'nontemporal->'.

Apply this fix:

         self.specific_share_to_other_effects_invest = flow_system.fit_effects_to_model_coords(
-            f'{prefix}|operation->',
+            f'{prefix}|nontemporal->',
             self.specific_share_to_other_effects_invest,
             'operation',

Note: Line 424 also needs updating to use the new mode parameter:

             self.specific_share_to_other_effects_invest,
-            'operation',
+            'nontemporal',

🧹 Nitpick comments (2)

flixopt/structure.py (1)

370-399: Tighten kwargs validation: bind to class init, avoid local import, sort keys

Functionality is good. Minor refinements for clarity and determinism:

• Bind signature to type(self).init (explicit class method).
• Use the top-level inspect import (remove local import).
• Sort unexpected keys for stable error messages.

-        import inspect
-
-        sig = inspect.signature(self.__init__)
+        # use top-level inspect import; bind to class for clarity
+        sig = inspect.signature(type(self).__init__)
         known_params = set(sig.parameters.keys()) - {'self', 'kwargs'}
         # Also filter out 'kwargs' itself which can appear during deserialization
         extra_kwargs = {k: v for k, v in kwargs.items() if k not in known_params and k != 'kwargs'}
 
         if extra_kwargs:
             class_name = class_name or self.__class__.__name__
-            unexpected_params = ', '.join(f"'{param}'" for param in extra_kwargs.keys())
+            unexpected_params = ', '.join(f"'{param}'" for param in sorted(extra_kwargs.keys()))
             raise TypeError(f'{class_name}.__init__() got unexpected keyword argument(s): {unexpected_params}')

flixopt/components.py (1)

1058-1087: Document strict kwargs validation or provide legacy aliases
Calling self._validate_kwargs now rejects unexpected kwargs; update the SourceAndSink docstring to call out this strict validation or add targeted aliases for the deprecated parameters.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95f3666 and 9ddc509.

📒 Files selected for processing (17)

• CHANGELOG.md (1 hunks)
• examples/03_Calculation_types/example_calculation_types.py (1 hunks)
• examples/05_Two-stage-optimization/two_stage_optimization.py (1 hunks)
• flixopt/calculation.py (1 hunks)
• flixopt/components.py (5 hunks)
• flixopt/effects.py (10 hunks)
• flixopt/elements.py (1 hunks)
• flixopt/features.py (6 hunks)
• flixopt/results.py (9 hunks)
• flixopt/structure.py (1 hunks)
• tests/test_effect.py (6 hunks)
• tests/test_flow.py (6 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)
• tests/test_scenarios.py (3 hunks)

🔇 Additional comments (49)

flixopt/components.py (2)

1343-1353: No Sink instantiations with uncommon kwargs—strict validation safe
rg found no Sink(...) calls using on_off_parameters or prevent_simultaneous_flows.

---

1212-1222: No component-only kwargs used in Source instantiations
Search found no calls to Source(...) with extra kwargs; deprecation warning and kwargs validation are safe.

flixopt/elements.py (1)

646-647: No action needed: add_share_to_effects supports ‘temporal’
Signature defines target: Literal['temporal','nontemporal'] and all aggregation/plots use the ‘temporal’ label.

tests/test_io.py (1)

56-59: LGTM! Consistent with the temporal/nontemporal refactoring.

The change from accessing aggregated costs via costs|total to costs aligns with the broader repository refactoring that renamed Effect domains from operation/invest to temporal/nontemporal. Based on the flixopt documentation, the framework "abstracts costs as so called 'Effects'. This allows to model costs, CO2-emissions, primary-energy-demand or area-demand at the same time." The updated assertions correctly reflect the new public API structure.

examples/03_Calculation_types/example_calculation_types.py (2)

210-210: LGTM! Updated plotting key aligns with temporal refactoring.

The change from costs(operation)|total_per_timestep to costs(temporal)|per_timestep correctly reflects the domain renaming and the simplified per-timestep naming convention. This ensures plotting functionality remains consistent with the new API.

---

217-217: LGTM! Consistent plotting key update.

Similar to the previous change, this update maintains consistency with the temporal/nontemporal refactoring across all plotting references.

flixopt/calculation.py (1)

110-111: LGTM! Effect submodel field names updated correctly.

The renaming from operation and invest to temporal and nontemporal provides clearer semantic meaning for the Effect domains. The temporal domain captures time-varying effects (like operational costs per timestep), while nontemporal captures time-invariant effects (like investment costs). This change aligns with the broader refactoring across the codebase.

tests/test_scenarios.py (3)

242-242: LGTM! Objective expression updated to use normalized variable names.

The change from costs|total to costs reflects the normalized variable naming in the optimization model. This maintains consistency with the broader refactoring while preserving the correct weighted objective calculation logic.

---

253-253: LGTM! Consistent objective expression update.

This change mirrors the previous update, ensuring all scenario tests use the consistent variable naming convention.

---

328-328: LGTM! Test assertion updated with normalized variable names.

The change from costs|total to costs in the test assertion maintains consistency with the model variable renaming while preserving the test logic for objective value verification.

tests/test_functional.py (3)

115-115: LGTM! Total costs assertion updated correctly.

The change from costs|total to costs aligns with the normalized variable naming in the solution results. This maintains test correctness while adopting the new API structure.

---

125-125: LGTM! Per-timestep costs variable updated appropriately.

The change from costs(operation)|total_per_timestep to costs(temporal)|per_timestep correctly reflects both the domain rename (operation→temporal) and the simplified timestep naming convention. This maintains test accuracy while adopting the new terminology.

---

132-132: LGTM! Component-specific cost variable updated consistently.

The change from costs(operation) to costs(temporal) for the Gas tariff cost variable maintains consistency with the temporal domain renaming across all test assertions.

tests/test_linear_converter.py (2)

187-191: LGTM! Consistent test update for temporal effects.

The renaming from operation to temporal in the constraint names is correctly applied here and maintains consistency with the domain renaming throughout the codebase.

---

490-495: LGTM! Temporal effects consistently updated.

The constraint and variable names have been properly updated to use the new temporal domain terminology.

tests/test_flow.py (5)

125-138: LGTM! Effects per flow hour correctly updated.

The test assertions for effects per flow hour have been properly updated to use temporal domain terminology, maintaining consistency with the new naming scheme.

---

470-486: LGTM! Investment effects properly migrated.

The investment effects have been correctly renamed to use nontemporal domain terminology, aligning with the PR's objective to rename invest to nontemporal.

---

507-513: LGTM! Divestment effects consistently updated.

The divestment effects constraint has been properly updated to use the nontemporal domain.

---

622-641: LGTM! Per-running-hour effects updated to temporal.

The effects per running hour have been correctly renamed to use the temporal domain terminology.

---

1025-1031: LGTM! Startup cost effects properly migrated.

The startup cost effects have been correctly updated to use the temporal domain, as these are operation-related costs.

tests/test_integration.py (6)

66-71: LGTM! Top-level effect names simplified as expected.

The test correctly reflects the simplified naming where costs|total becomes just costs and CO2|total becomes just CO2 in the results.

---

85-98: LGTM! Per-timestep aggregation correctly renamed.

The test properly updates from costs(operation)|total_per_timestep to costs(temporal)|per_timestep, following the new naming convention.

---

101-129: LGTM! Component-level temporal effects consistently updated.

All component-specific cost contributions have been correctly renamed from operation to temporal domain.

---

132-141: LGTM! Investment effects renamed to nontemporal.

The investment-related costs for components have been properly updated to use the nontemporal domain terminology.

---

144-152: LGTM! Effect totals properly split between domains.

The test correctly validates that CO2 emissions are split between CO2(temporal) and CO2(nontemporal) domains.

---

307-316: Document segmented calculation handling difference
Aggregated models use results.model['costs'].solution.item(), segmented models use solution_without_overlap('costs(temporal)|per_timestep').sum()—confirm this divergence is intentional and add a brief note in the test or documentation.

tests/test_effect.py (6)

26-34: LGTM! Effect variable structure properly updated.

The test correctly validates the new variable structure with separate temporal and nontemporal components plus the combined total effect.

---

36-45: LGTM! Constraint structure matches variable structure.

The constraints mirror the variable structure with proper domain separation.

---

59-72: LGTM! Effect composition correctly implemented.

The test properly validates that the total effect equals the sum of temporal and nontemporal components.

---

80-88: LGTM! Effect bounds properly migrated to new parameter names.

The test correctly uses the renamed parameters:

• minimum/maximum_operation → minimum/maximum_temporal
• minimum/maximum_investment → minimum/maximum_nontemporal
• minimum/maximum_operation_per_hour → minimum/maximum_per_hour

---

244-262: LGTM! Effect share factors properly separated by domain.

The test correctly validates effect share factors for both temporal and nontemporal domains.

---

283-303: LGTM! Component effect aggregation validated for all domains.

The test properly validates that effects per component sum correctly for temporal, nontemporal, and total domains.

flixopt/features.py (5)

100-100: LGTM! Consistent refactoring to temporal/nontemporal terminology.

The changes from invest to nontemporal in the target parameter for add_share_to_effects calls are consistent and align with the PR's objective to rename Effect domains. The terminology change makes the distinction between time-dependent (temporal) and time-independent (nontemporal) effects clearer.

Also applies to: 110-110, 117-117

---

243-243: LGTM! Consistent updates in OnOffModel effects.

The target parameter changes from operation to temporal for the OnOff model's runtime-dependent effects are appropriate and maintain consistency with the new naming convention.

Also applies to: 252-252

---

487-487: LGTM! PiecewiseEffectsModel updated correctly.

The target parameter change to nontemporal for piecewise investment effects aligns with the PR objectives.

---

529-533: Clean refactoring of ShareAllocationModel naming.

The changes improve clarity and consistency:

• Using self.label_full directly for the total variable name
• Renaming total_per_timestep to per_timestep removes redundancy
• Consistent constraint naming throughout

These changes align with the broader temporal/nontemporal refactoring effort.

Also applies to: 540-544

---

591-591: Verify constraint reference update.

The update from _eq_total_per_timestep to self._eq_total_per_timestep is correct since the constraint is now stored as an instance variable.

flixopt/results.py (6)

286-286: LGTM! Effect share factors updated to new naming.

The dictionary keys have been correctly updated from operation/invest to temporal/nontemporal.

---

359-359: LGTM! Effects dataset modes updated consistently.

The loop now correctly iterates over ['temporal', 'nontemporal', 'total'] instead of the old naming.

---

476-491: Well-implemented backwards-compatible API update.

The get_effect_shares method signature and implementation have been properly updated:

• Mode parameter now accepts 'temporal'/'nontemporal' instead of operation/invest terminology
• Documentation correctly describes the new modes
• Error message provides clear guidance on valid options
• Recursive calls use the new parameter names

Also applies to: 507-510, 515-516

---

544-589: Comprehensive update to _compute_effect_total method.

The implementation correctly handles the new temporal/nontemporal terminology:

• Method signature updated with new mode literals
• Documentation reflects the new terminology
• Internal logic properly computes temporal and nontemporal components
• Null checking and summation logic preserved correctly

---

621-627: LGTM! Consistent updates to effects dataset creation.

The _create_effects_dataset method correctly uses the new mode terminology in both the signature and documentation.

---

672-674: Suffix mapping updated correctly.

The suffix mapping dictionary properly maps the new mode names to their corresponding suffixes:

• 'temporal' → '(temporal)|per_timestep'
• 'nontemporal' → '(nontemporal)'
• 'total' → ''

This ensures result labels remain consistent with the new terminology.

flixopt/effects.py (6)

57-66: Well-executed deprecation handling for backwards compatibility.

The implementation provides excellent backwards compatibility through:

1. Clear deprecation warnings with actionable messages
2. Validation to prevent conflicting parameter usage
3. Comprehensive parameter mapping from old to new names
4. Proper kwargs validation to catch unexpected parameters

The deprecation strategy ensures existing code continues to work while guiding users toward the new API.

Also applies to: 163-268

---

270-390: Clean implementation of deprecated property accessors.

The deprecated properties provide a smooth migration path:

• Both getters and setters emit clear deprecation warnings
• Properties correctly proxy to the new internal attributes
• Warning messages guide users to the new property names

This approach maintains full backwards compatibility while encouraging migration.

---

446-481: Clean refactoring of EffectModel submodels.

The EffectModel correctly:

• Renames submodels to nontemporal and temporal
• Updates label formatting with new terminology
• Properly maps min/max parameters to the renamed attributes
• Maintains the total calculation as sum of temporal and nontemporal

---

576-587: Clear error messages for cycle detection.

The cycle detection has been properly updated to:

• Check temporal and nontemporal shares separately
• Provide clear, specific error messages for each type of cycle

---

693-709: Comprehensive updates to share allocation methods.

Both add_share_to_effects and _add_share_between_effects have been correctly updated:

• Target parameter accepts new 'temporal'/'nontemporal' literals
• Shares are properly routed to the correct submodels
• Cross-effect shares correctly reference the renamed submodels

Also applies to: 733-746

---

660-676: LGTM! Effect share factor calculation updated.

The calculate_effect_share_factors method now correctly:

• Returns temporal and nontemporal share dictionaries
• Processes shares with the updated attribute names
• Maintains the same calculation logic with new naming

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Confirm exact legacy parameter names ('minimum_invest' vs 'minimum_investment')

The changelog lists minimum_investment/maximum_investment; earlier notes sometimes reference minimum_invest/maximum_invest. Ensure the deprecation mapping and docs reflect the actual legacy names used in the API.

---

🏁 Script executed:

#!/bin/bash
# Check for occurrences of both variants to ensure consistency
rg -nP "(minimum|maximum)_(invest(ment)?|operation(_per_hour)?)" --type=py -C2
rg -nP "(minimum|maximum)_(nontemporal|temporal|per_hour)" --type=py -C2

Length of output: 33793

---

Use correct legacy names (minimum_invest/maximum_invest) in CHANGELOG.md
Change the entries from minimum_investment/maximum_investment to minimum_invest/maximum_invest to match the actual deprecated parameters.

🤖 Prompt for AI Agents

In CHANGELOG.md around lines 111 to 117, the legacy parameter names are
incorrect: update the listed renamed Effect parameters from
`minimum_investment`/`maximum_investment` to the actual deprecated names
`minimum_invest`/`maximum_invest` so the changelog matches the real legacy
parameter names; keep the other renames unchanged and verify spelling and
backticks for consistency.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Aligns with renamed effect domains; please verify keys exist in results

The switch to 'costs', 'costs(nontemporal)', and 'costs(temporal)' matches the new terminology. Please confirm these variables are present in results for both Combined and Two-stage modes.

---

🏁 Script executed:

#!/bin/bash
# Inspect available data variables in both solutions to verify keys
rg -nP -C2 "results\.solution" --type=py
# Optionally print example outputs if a small fixture exists
rg -nP "costs\(temporal\)|costs\(nontemporal\)" --type=py -C2

Length of output: 20492

---

Update temporal cost key in example Replace 'costs(temporal)' with 'costs(temporal)|per_timestep' in examples/05_Two-stage-optimization/two_stage_optimization.py to match results.solution keys.

🤖 Prompt for AI Agents

In examples/05_Two-stage-optimization/two_stage_optimization.py around lines 148
to 150, the example uses the outdated key 'costs(temporal)'; update that entry
to 'costs(temporal)|per_timestep' so it matches the keys present in
results.solution (i.e., replace 'costs(temporal)' with
'costs(temporal)|per_timestep').