Feature/properties

[FBumann]

Description

Brief description of the changes in this PR.

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

• Documentation

  • Added a comprehensive cluster-architecture design doc describing true cluster/time dimensions, plotting, per-cluster aggregation, and a phased rollout roadmap.

• Improvements

  • Broadly converted many public attributes to guarded properties (immutable labels, metadata/color updates trigger reprocessing, cluster-aware options, prevent-simultaneous-flow control), and standardized invalidation behavior across components.
  • Startup counting now respects cluster weighting.

• Tests

  • Added extensive tests for property guards, invalidation, copy-semantics, and re-optimization flow.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@FBumann has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 25 minutes and 17 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between fda20c9 and 3fd432f.

📒 Files selected for processing (7)

• flixopt/components.py
• flixopt/effects.py
• flixopt/elements.py
• flixopt/interface.py
• flixopt/linear_converters.py
• flixopt/structure.py
• tests/test_property_guards.py

📝 Walkthrough

Walkthrough

Refactors core classes to use private backing fields with property accessors and invalidation across components, flows, effects, invest parameters, and structure; adds a cluster-architecture design doc and tests; adjusts startup counting (cluster weighting) and internal linking to operate on backing fields.

Changes

Cohort / File(s)	Summary
Design & Architecture docs/design/cluster_architecture.md	New design document describing true (cluster, time) dimensions, Option B approach, model impacts, plotting behavior, phased implementation roadmap, testing strategy, and API/backwards-compat considerations.
Core elements & models flixopt/elements.py, flixopt/components.py, flixopt/structure.py, flixopt/flow_system.py	Convert many public attributes to private backing fields (_...) with property getters/setters that call _invalidate(); internal flows updated to read/write backing fields directly to avoid spurious invalidation. Element label made immutable; meta_data/color now invalidate. Connection wiring assigns internal fields.
Effects & Features flixopt/effects.py, flixopt/features.py	Effect class moved to backing-field-backed properties for all temporal/periodic constraints and effect parameters with invalidation on mutation; transform_data uses backing fields. StatusModel startup counting now applies cluster weighting when startup_limit is used.
Investment API flixopt/interface.py	InvestParameters refactored to private backing fields with full property accessors (effects_of_investment, effects_of_retirement, fixed_size, piecewise effects, min/max sizes, linked_periods, mandatory, etc.); transform_data updated to operate on backing fields; Interface._invalidate added.
Converters & Linear components flixopt/linear_converters.py	Converters (Boiler, Power2Heat, HeatPump, HeatPumpWithSource, CoolingTower, CHP) now use _conversion_factors and property setters that call _invalidate(); COP/efficiency accessors updated accordingly.
System integration & plumbing flixopt/flow_system.py, flixopt/features.py	_assign_element_colors_ and _connect_network_ updated to use backing fields for colors, inputs/outputs, and flow→component linking to prevent unintended invalidation during setup; _invalidate_model note clarifies locking behavior.
Tests tests/test_property_guards.py	New comprehensive test suite validating immutability, invalidation behavior pre/post-linking, copy semantics for collections, property guard behavior across flows/components/effects/invest/storage, and re-optimization after changes.

Sequence Diagram(s)

(omitted — changes are broad refactors, docs, and tests without a single new multi-component runtime control-flow that benefits from a focused sequence diagram)

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• Feature/v3/feature/308 rename effect domains #365: Modifies effects.py and Effect internals — closely overlaps with the Effect backing-field/property changes in this PR.
• Change default size to be None. If mathematically needed, Users need to specify it #517: Changes InvestParameters and sizing semantics — directly related to the InvestParameters refactor here.
• Invalidate and lock FLow system based on solution existance #518: Modifies linkage and transform APIs (link_to_flow_system / transform_data) — closely connected to backing-field-based transform/link updates.

Poem

🐰 I tuck my fields with underscores neat,
Properties guard each tuneful beat,
Invalidate when values change,
Linking flows now alters range,
Clusters sprout and tests applaud—🥕

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is entirely a template with no actual content filled in; all sections (Description, Type of Change, Related Issues, Testing) lack specific information about the changes or their rationale.	Complete the description template with a clear summary of the changes, specify the type of change, reference any related issues, and confirm testing status with concrete details.
Docstring Coverage	⚠️ Warning	Docstring coverage is 43.86% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'Feature/properties' is vague and generic, using non-descriptive terminology that does not clearly convey the specific nature or scope of the changes in the changeset.	Provide a more descriptive title that specifies the main change, such as 'Refactor public attributes to property-based accessors with invalidation' or similar.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.}

[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/components.py (1)

1907-1923: prevent_simultaneous_flow_rates flag does not update the modeled mutual‑exclusion constraint when changed after init

For SourceAndSink, Source, and Sink:

• The constructor wires mutual exclusivity into the base Component via prevent_simultaneous_flows=... based on the initial prevent_simultaneous_flow_rates value.
• The prevent_simultaneous_flow_rates attribute/property then only updates a boolean backing field and calls _invalidate() (for Source/Sink). It does not update Component._prevent_simultaneous_flows.

As a result, toggling prevent_simultaneous_flow_rates after construction (and even after re‑optimization) does not actually add/remove the mutual‑exclusion constraint in ComponentModel; the model continues to use whatever prevent_simultaneous_flows list was set at construction time.

Given the new property/invalidaton pattern, this is surprising behavior and can lead to silent mismatches between API flags and actual model constraints.

I suggest wiring the boolean property into the underlying flow list so that future (re)optimizations see the change, e.g.:

Proposed fix for `Source`, `Sink`, and `SourceAndSink`

@@ class SourceAndSink(Component):
-    def __init__(
+    def __init__(
         self,
         label: str,
         inputs: list[Flow] | None = None,
         outputs: list[Flow] | None = None,
         prevent_simultaneous_flow_rates: bool = True,
         meta_data: dict | None = None,
     ):
-        super().__init__(
+        super().__init__(
             label,
             inputs=inputs,
             outputs=outputs,
             prevent_simultaneous_flows=(inputs or []) + (outputs or []) if prevent_simultaneous_flow_rates else None,
             meta_data=meta_data,
         )
-        self.prevent_simultaneous_flow_rates = prevent_simultaneous_flow_rates
+        self._prevent_simultaneous_flow_rates = prevent_simultaneous_flow_rates
+
+    @property
+    def prevent_simultaneous_flow_rates(self) -> bool:
+        """If True, prevent simultaneous nonzero input and output flows."""
+        return self._prevent_simultaneous_flow_rates
+
+    @prevent_simultaneous_flow_rates.setter
+    def prevent_simultaneous_flow_rates(self, value: bool) -> None:
+        self._prevent_simultaneous_flow_rates = value
+        # Keep underlying mutual‑exclusion list in sync
+        self._prevent_simultaneous_flows = (self._inputs + self._outputs) if value else []
+        self._invalidate()
@@ class Source(Component):
-    def __init__(
+    def __init__(
         self,
         label: str,
         outputs: list[Flow] | None = None,
         meta_data: dict | None = None,
         prevent_simultaneous_flow_rates: bool = False,
     ):
-        self._prevent_simultaneous_flow_rates = prevent_simultaneous_flow_rates
+        self._prevent_simultaneous_flow_rates = prevent_simultaneous_flow_rates
         super().__init__(
             label,
             outputs=outputs,
             meta_data=meta_data,
             prevent_simultaneous_flows=outputs if prevent_simultaneous_flow_rates else None,
         )
@@
     @prevent_simultaneous_flow_rates.setter
     def prevent_simultaneous_flow_rates(self, value: bool) -> None:
-        self._prevent_simultaneous_flow_rates = value
-        self._invalidate()
+        self._prevent_simultaneous_flow_rates = value
+        # Update underlying Component mutual‑exclusion wiring
+        self._prevent_simultaneous_flows = list(self._outputs) if value else []
+        self._invalidate()
@@ class Sink(Component):
-        self._prevent_simultaneous_flow_rates = prevent_simultaneous_flow_rates
+        self._prevent_simultaneous_flow_rates = prevent_simultaneous_flow_rates
         super().__init__(
             label,
             inputs=inputs,
             meta_data=meta_data,
             prevent_simultaneous_flows=inputs if prevent_simultaneous_flow_rates else None,
         )
@@
     @prevent_simultaneous_flow_rates.setter
     def prevent_simultaneous_flow_rates(self, value: bool) -> None:
-        self._prevent_simultaneous_flow_rates = value
-        self._invalidate()
+        self._prevent_simultaneous_flow_rates = value
+        # Update underlying Component mutual‑exclusion wiring
+        self._prevent_simultaneous_flows = list(self._inputs) if value else []
+        self._invalidate()

This keeps the user‑visible boolean in sync with the actual constraints that ComponentModel builds, while preserving the backing‑field and invalidation patterns.

Also applies to: 2001-2024, 2104-2137

🤖 Fix all issues with AI agents

In @flixopt/elements.py:
- Around line 685-693: The getter effects_per_flow_hour currently
unconditionally calls dict(self._effects_per_flow_hour) which will raise if
_effects_per_flow_hour is a scalar or xarray.DataArray; modify the
effects_per_flow_hour property to only return dict(self._effects_per_flow_hour)
when isinstance(self._effects_per_flow_hour, dict), otherwise return
self._effects_per_flow_hour unchanged; mirror the pattern used in
InvestParameters/StatusParameters getters and ensure you still return a copy for
dict inputs while preserving scalar/array types.

In @flixopt/structure.py:
- Around line 438-447: The current _invalidate() on Interface calls
self._flow_system._invalidate_model() even when the FlowSystem is locked, which
can leave fs.model None but fs.solution/non-empty and fs.is_locked True,
violating the “locked cannot be modified” docstring; update the code so either
(a) FlowSystem._invalidate_model() clears the existing solution (e.g., reset
_solution and set is_locked False) when invoked from Interface invalidation, or
(b) change Interface._invalidate() to first check self._flow_system.is_locked
and either no-op or raise AttributeError instructing callers to call
FlowSystem.reset() before mutating; ensure you reference the FlowSystem class,
its _invalidate_model method, the Interface._invalidate method, the is_locked
property, and the solution/_solution field when making the change and add a
short docstring note describing the chosen behavior.

🧹 Nitpick comments (6)

flixopt/features.py (1)

237-242: Cluster-weighted startup counting looks correct; consider clarifying non-clustered semantics

The new startup_count formulation correctly:

• sums over all temporal dimensions except period/scenario, and
• applies cluster weights only, so that a startup in a heavier cluster represents more real startups.

Two minor points you may want to document inline:

• In non-clustered systems, self._model.weights has no 'cluster' key, so the multiplier falls back to 1.0. If cluster_weight were ever set without a cluster dimension, this code intentionally ignores it (unlike temporal_weight), which seems fine but is worth making explicit.
• For clustered systems with non-uniform timestep_duration, startup_count differs from any effect based on sum_temporal(self.startup), which uses timestep_duration * cluster_weight. If you rely on those two quantities to align, a short comment noting this deliberate difference would help future readers.

flixopt/linear_converters.py (1)

104-113: Backed conversion-factor properties and invalidation pattern look consistent; consider minor robustness tweaks

The refactor to have Boiler, Power2Heat, HeatPump, CoolingTower, CHP, and HeatPumpWithSource:

• read from _conversion_factors[idx].get(<input_label>), and
• write {input_label: value, output_label: 1} (or {electrical: -1, thermal: sed} for CoolingTower) followed by _invalidate()

is consistent with the documented conversion relationships and with the new backing-field/invalidation model.

If you want to harden this further:

• Guard getters against malformed internal state, e.g. by returning a default or raising a clear error when _conversion_factors is empty or missing the expected key, instead of letting IndexError/None leak out.
• For HeatPump and HeatPumpWithSource, you may optionally assert that _conversion_factors has exactly one (or two) entries when setting, to catch accidental external mutations earlier.

These are not required for correctness given the current constructors, but would make the API more defensive against future changes.

Also applies to: 201-209, 297-305, 393-401, 515-536, 636-649

flixopt/flow_system.py (1)

1172-1175: Correct use of backing fields in wiring; note pre-existing bus membership caveat

The switch in:

• _assign_element_colors to write component._color directly, and
• _connect_network to use component._inputs/_outputs, mutate flow._component / flow._is_input_in_component, and append into bus._outputs/_inputs

is exactly what you need now that the public list properties return copies and setters trigger invalidation. This prevents both no-op mutations and accidental _invalidate() calls during internal wiring.

One behavioral nuance to be aware of (already present before this refactor):

• _connect_network() only adds flows to bus._inputs/_outputs if they’re missing, but never removes flows that might have been associated with a different bus earlier. If you ever support changing a flow.bus after initial connection and relying on a subsequent _connect_network() to move it, you’ll need an explicit clear/rebuild of bus-side lists.

Not a blocker, but something to keep in mind if dynamic rewiring becomes more common.

Also applies to: 1835-1851

tests/test_property_guards.py (1)

21-326: Comprehensive property/invalidation tests; consider solver-cost tradeoff

This suite does a thorough job of pinning down the new semantics:

• Label immutability across all Element types.
• Before/after-linking behavior for flows, components, effects, and nested parameter objects.
• List/dict properties returning copies.
• Storage and HeatPump-specific guards.
• A full “optimize → modify → re-optimize” path that asserts both invalidation and changed objective.

The only minor tradeoff is the use of a real HighsSolver in TestFullWorkflow; the model is tiny so it’s likely fine, but if test runtime ever becomes a concern you could consider a lighter-weight solver or marking that test as “slow”. Functionally though, this is solid coverage.

Also applies to: 331-437

docs/design/cluster_architecture.md (1)

36-45: Documentation looks solid; fix minor Markdown/wording nits

The design doc reads well and aligns with the implemented (cluster, time) approach. Two tiny polish items from linters:

• The “Current Implementation” code block (around line 37) uses bare `````; adding a language (```python or ` ```text `) will satisfy markdownlint.
• The phrase “true (cluster, time) shaped data” would be more idiomatic as “true (cluster, time)-shaped data”.

Purely cosmetic; the technical content is fine as-is.

Also applies to: 394-404

flixopt/components.py (1)

87-107: Dynamic rewiring of flows can leave _flows and flow metadata inconsistent

Component.__init__ wires _inputs/_outputs into _flows and _connect_flows() once, but the inputs, outputs, and flows setters only mutate the backing lists/dict and call _invalidate().

If callers later reassign component.inputs / component.outputs / component.flows, you end up with:

• Flow._component / Flow._is_input_in_component still reflecting the old wiring.
• self._flows potentially stale if only inputs or outputs were changed.
• Model construction (ComponentModel/BusModel) using _inputs/_outputs, while other code (e.g. LinearConverterModel via self.element.flows[...]) uses _flows.

This makes topology changes after initialization fragile and can cause mismatches or attribute errors when new flows lack submodels.

I recommend, if you want to support changing flows post‑construction, to have these setters also:

• Recompute _flows from _inputs + _outputs.
• Re-run _check_unique_flow_labels() and _connect_flows() (or a lighter variant) to refresh Flow._component / Flow._is_input_in_component.
• Then _invalidate().

If dynamic rewiring is deliberately unsupported, consider documenting that and possibly making the setters internal‑only to avoid misleading users.

Also applies to: 110-159, 186-235

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 71bca89 and b1cbab6.

📒 Files selected for processing (10)

• docs/design/cluster_architecture.md
• flixopt/components.py
• flixopt/effects.py
• flixopt/elements.py
• flixopt/features.py
• flixopt/flow_system.py
• flixopt/interface.py
• flixopt/linear_converters.py
• flixopt/structure.py
• tests/test_property_guards.py

🧰 Additional context used

🧬 Code graph analysis (5)

flixopt/flow_system.py (1)

flixopt/elements.py (6)

• component (776-778)
• component (781-783)
• is_input_in_component (786-788)
• is_input_in_component (791-793)
• bus (636-638)
• bus (641-643)

flixopt/features.py (2)

flixopt/flow_system.py (2)

• dims (1947-1967)
• weights (2126-2146)

flixopt/structure.py (2)

• dims (229-231)
• weights (239-244)

flixopt/structure.py (1)

flixopt/flow_system.py (2)

• _invalidate_model (1491-1512)
• FlowSystem (55-2521)

flixopt/interface.py (4)

flixopt/structure.py (7)

• _invalidate (438-446)
• prefix (391-393)
• _fit_effect_coords (484-502)
• dims (229-231)
• transform_data (375-388)
• _fit_coords (469-482)
• flow_system (449-467)

flixopt/flow_system.py (1)

• dims (1947-1967)

flixopt/components.py (3)

• transform_data (243-250)
• transform_data (671-710)
• transform_data (1054-1058)

flixopt/effects.py (1)

• transform_data (402-445)

flixopt/effects.py (2)

flixopt/structure.py (5)

• _invalidate (438-446)
• _fit_coords (469-482)
• prefix (391-393)
• _fit_effect_coords (484-502)
• dims (229-231)

flixopt/flow_system.py (1)

• dims (1947-1967)

🪛 LanguageTool

docs/design/cluster_architecture.md

[grammar] ~512-~512: Use a hyphen to join words.
Context: ...Goal: Produce true (cluster, time) shaped data. Tasks: 1. Update `clus...

(QB_NEW_EN_HYPHEN)

🪛 markdownlint-cli2 (0.18.1)

docs/design/cluster_architecture.md

37-37: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

⏰ 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). (5)

• GitHub Check: test (3.14)
• GitHub Check: test (3.13)
• GitHub Check: test (3.12)
• GitHub Check: test (3.11)
• GitHub Check: Build documentation

🔇 Additional comments (2)

flixopt/effects.py (1)

209-239: Effect backing fields + properties are consistent and safe for invalidation

The refactor to back Effect fields with private attrs (_unit, _description, _is_standard, all min/max bounds, share maps, and period_weights) and to:

• Expose them via properties that invalidate on mutation, and
• Use the backing fields directly in transform_data() to avoid recursive invalidation,

is internally consistent and aligns with the patterns in Flow/InvestParameters/StatusParameters. The copy‑on‑read behavior for share_from_temporal/share_from_periodic and effect dictionaries also protects against accidental in‑place mutation of internal state.

I don’t see correctness or dimension issues here; this looks good.

Also applies to: 240-394, 402-445

flixopt/interface.py (1)

1146-1262: InvestParameters/StatusParameters property refactor and transform_data behavior look consistent

The move to private backing fields with property accessors for InvestParameters (effects, sizes, piecewise, linked_periods) and StatusParameters (effects, uptime/downtime, active hours, startup_limit, cluster_mode), combined with:

• A single validation in InvestParameters.transform_data() that enforces fixed_size or maximum_size to be set, and
• transform_data() operating directly on the backing fields via _fit_coords / _fit_effect_coords,

is coherent and matches the design elsewhere (Effect, Flow, Storage).

The new upper‑bound requirement on investments is a behavioral tightening but is well‑motivated by scaling constraints and is centrally enforced; downstream uses via minimum_or_fixed_size / maximum_or_fixed_size look correct.

No issues from a correctness or modeling perspective.

Also applies to: 1282-1346, 1577-1750