Feature/rename investparameter optional to mandatory

[FBumann]

Description

Closes #358

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

  • Introduced a mandatory flag for investment parameters, providing explicit control over required vs. optional investments.
  • Added backward-compatible optional alias that emits deprecation warnings guiding migration to mandatory.

• Refactor

  • Unified investment logic to rely on the mandatory flag, affecting size initialization, lower-bound handling (0 when not mandatory), and fixed-size activation (requires mandatory).
  • Updated divestment behavior to depend on not mandatory, aligning effects with the new flag.

[coderabbitai]

Walkthrough

Refactors investment logic to use a mandatory flag instead of optional across InvestParameters, Flow.flow_rate_lower_bound, and InvestmentModel control paths. Adds a deprecated optional alias with warnings for backward compatibility. Updates divest and size handling to rely on mandatory. Removes Flow.invest_is_optional.

Changes

Cohort / File(s)	Summary
InvestParameters API refactor flixopt/interface.py	Introduced mandatory flag; added deprecated optional alias with getter/setter mapping to inverse of mandatory and deprecation warnings; adjusted __init__ signature and backward-compatibility logic.
Flow lower bound semantics flixopt/elements.py	Removed Flow.invest_is_optional; flow_rate_lower_bound now uses mandatory on InvestParameters: if not mandatory → lower bound 0; else compute from relative and size bounds.
Investment modeling control flow flixopt/features.py	Replaced optional checks with mandatory; fixed-size branch requires fixed_size and mandatory; non-mandatory yields lower=0; updated divest/share creation logic to depend on mandatory.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant U as User Code
    participant IP as InvestParameters
    participant IM as InvestmentModel
    participant F as Flow

    rect rgb(236, 248, 255)
    note over U,IP: Initialization with new flag
    U->>IP: __init__(..., mandatory, optional=None)
    alt optional provided (deprecated)
        IP-->>U: DeprecationWarning (use mandatory)
        IP->>IP: Set mandatory = not optional
    else mandatory provided
        IP->>IP: Store mandatory
    end
    end

    rect rgb(244, 240, 255)
    note over U,IM: Modeling based on mandatory
    U->>IM: do_modeling(IP)
    alt fixed_size and mandatory
        IM->>IM: Apply fixed-size constraints
    else not mandatory
        IM->>IM: Use lower bounds = 0
        IM->>IM: Skip mandatory-only divest effects
    else mandatory (min/max)
        IM->>IM: Use minimum_size for lower bounds
        IM->>IM: Apply divest/specific effects
    end
    end

    rect rgb(235, 255, 242)
    note over IM,F: Flow bounds
    IM->>F: Compute flow_rate_lower_bound(IP)
    alt IP.mandatory == false
        F-->>IM: lower_bound = 0
    else
        F-->>IM: lower_bound = relative * size bound
    end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Feature/rename investparameter optional to mandatory #358: Performs the same rename from optional→mandatory and aligns Flow and features logic with the new flag, including removal of Flow.invest_is_optional.

Poem

A nibble of flags in the meadow of code,
Optional hops flipped to mandatory mode.
Bounds now settle where logic is clear,
Warnings squeak softly: “the new path is here.”
I twitch my whiskers—refactor complete! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The description section only states “Closes #358” without providing a summary of the actual code changes, leaves the Type of Change checkboxes unset, repeats a placeholder under Related Issues, and omits confirmation of testing steps, so it does not follow the required template.	Please add a brief description of the changes under “Description,” select the appropriate Type of Change checkbox, fill in the actual related issue number under Related Issues, and mark the testing section once tests have been executed.
Docstring Coverage	⚠️ Warning	Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title accurately summarizes the primary change—renaming the investment parameter from optional to mandatory—and clearly reflects the core refactoring performed in the pull request. It is concise and directly related to the main change.
Linked Issues Check	✅ Passed	All code modifications directly implement the objectives of issue #358 by renaming the “optional” parameter to “mandatory,” adding deprecation warnings, refactoring flow bounds and investment modeling to respect the new flag, and removing the old invest_is_optional property.
Out of Scope Changes Check	✅ Passed	All changes pertain solely to the optional-to-mandatory refactoring and deprecation requirements specified in the linked issue, with no unrelated code modifications detected.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature/rename-investparameter-optional-to-mandatory

---

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: 0

🧹 Nitpick comments (1)

flixopt/interface.py (1)

852-864: Remove redundant import statement.

The import warnings on line 855 inside the property getter is unnecessary since warnings is already imported at the module level (line 9). This causes a redundant import on every property access.

Apply this diff to use the module-level import:

 @property
 def optional(self) -> bool:
     """DEPRECATED: Use 'mandatory' property instead. Returns the opposite of 'mandatory'."""
-    import warnings
-
     warnings.warn("Property 'optional' is deprecated. Use 'mandatory' instead.", DeprecationWarning, stacklevel=2)
     return not self.mandatory

Otherwise, the deprecation implementation is well-designed:

• Both getter and setter emit warnings
• stacklevel=2 correctly points to the caller
• Semantic mapping (optional = not mandatory) is correct

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 834f44a and 4eefd29.

📒 Files selected for processing (3)

• flixopt/elements.py (1 hunks)
• flixopt/features.py (3 hunks)
• flixopt/interface.py (6 hunks)

🔇 Additional comments (7)

flixopt/elements.py (1)

648-650: LGTM! Correctly uses the new mandatory flag.

The logic properly handles the renamed parameter:

• When not mandatory (i.e., optional), the lower bound is 0 (investment can be skipped)
• When mandatory, the minimum_size constraint applies

This aligns with the deprecation of optional in favor of mandatory across the codebase.

flixopt/features.py (4)

49-55: LGTM! Fixed-size mandatory investment logic is correct.

The condition properly requires both fixed_size and mandatory to be true, which correctly models a forced investment at a specific size.

---

56-64: LGTM! Size bounds correctly reflect mandatory status.

When not mandatory (optional), the lower bound is 0 allowing the optimizer to skip the investment. When mandatory, the minimum_size constraint applies. This correctly implements the new semantics.

---

66-72: LGTM! Optional investment handling updated correctly.

The comment clarifies that "Optional (not mandatory)" creates the is_invested binary variable, which is only needed when the investment is not mandatory. The logic correctly uses not self.parameters.mandatory.

---

92-101: LGTM! Divestment logic correctly restricted to optional investments.

Divest effects (penalties for not investing) only make sense when the investment is optional. When mandatory, there's no choice, so divest effects appropriately do not apply.

flixopt/interface.py (2)

9-9: LGTM! Module-level warnings import added.

This is needed for the deprecation warnings in the optional property getter/setter.

---

826-845: LGTM! Backwards-compatible parameter refactoring.

The new mandatory parameter with default False maintains backwards compatibility (investments are optional by default). The handling of the deprecated optional parameter correctly delegates to the property setter, which will emit appropriate warnings and set mandatory to the inverse value.

[FBumann]

Closed in favor of #392