Skip to content

Feature/rename investparameter optional to mandatory - #358

Merged
FBumann merged 3 commits into
v2.2/mainfrom
feature/rename-investparameter-optional-to-mandatory
Sep 27, 2025
Merged

Feature/rename investparameter optional to mandatory#358
FBumann merged 3 commits into
v2.2/mainfrom
feature/rename-investparameter-optional-to-mandatory

Conversation

@FBumann

@FBumannFBumann commented Sep 26, 2025

Copy link
Copy Markdown
Member

Description

Brief description of the changes in this PR.

Type of Change

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

Related Issues

#301

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” setting for investments, providing clearer control over required vs. optional sizing.
  • Deprecations
    • The legacy “optional” parameter/property is deprecated; using it triggers a deprecation warning and maps to the new “mandatory” behavior.
  • Refactor
    • Standardized sizing and divest behavior to depend on “mandatory” status: when not mandatory, lower bounds default to 0; when mandatory, minimum size applies. Fixed-size modeling now aligns with the mandatory flag.
  • Breaking Changes
    • Removed a public property indicating optional investment on flows.

@FBumannFBumann linked an issue Sep 26, 2025 that may be closed by this pull request
2 tasks
@coderabbitai

coderabbitaiBot commented Sep 26, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

The changes replace investment “optional” semantics with a “mandatory” flag across elements, features, and interface. Flow and feature logic now branch on mandatory. Interface adds a deprecated optional property for backward compatibility, updates InvestParameters’ constructor, and emits deprecation warnings when optional is used.

Changes

Cohort / File(s)Summary of changes
Mandatory flag adoption in core logic
flixopt/elements.py
Removed Flow.invest_is_optional. Updated Flow.flow_rate_lower_bound to use mandatory: if not mandatory, lower bound is 0; else uses minimum_size.
Feature flow control refactor
flixopt/features.py
Replaced fixed_size/optional gating with mandatory-based branching. Adjusted modeling, lower bound expression, investment creation, and divest logic to depend on mandatory. Updated comments accordingly.
API and compatibility layer
flixopt/interface.py
Added InvestParameters.mandatory field. Updated init signature to accept mandatory and an optional optional parameter for compatibility. Introduced deprecated optional property (getter/setter) mapping to inverse of mandatory with DeprecationWarning. Imported warnings and wired deprecation path.

Sequence Diagram(s)

sequenceDiagram
autonumber
actor Client
participant InvestParameters
participant Features as Features/Modeling
participant Flow
Client->>InvestParameters: Initialize(fixed_size, min, max, mandatory, ...[, optional?])
alt optional provided (deprecated)
InvestParameters->>InvestParameters: Set mandatory = not optional
InvestParameters-->>Client: DeprecationWarning
else mandatory provided
InvestParameters-->>Client: Store mandatory
end
Client->>Features: do_modeling(invest=InvestParameters)
alt fixed_size and mandatory
Features->>Features: Apply fixed sizing branch
else not fixed_size
Features->>Flow: flow_rate_lower_bound(mandatory, minimum_size)
alt not mandatory
Flow-->>Features: lower_bound = 0
else mandatory
Flow-->>Features: lower_bound = minimum_size
end
end
alt divest_effects present and not mandatory
Features->>Features: Execute divest_effects
end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I thump my paw: “Mandatory now!”
Optional’s moon takes a gentle bow.
Warnings squeak—deprecated breeze—
While bounds drop neatly to zero or keys.
I nibble specs, ears high with cheer,
Hop-hop—refactors crystal clear! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check nameStatusExplanationResolution
Description Check⚠️ WarningThe description section remains the template placeholder without any actual summary of the changes, the Related Issues line is missing the “Closes” keyword, and the testing section indicates that existing tests still need verification, so it lacks the required details.Please replace the placeholder description with a brief summary of your refactoring, update the Related Issues section to use “Closes #301,” and confirm that all existing tests still pass by checking the corresponding box.
Docstring Coverage⚠️ WarningDocstring 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 (1 passed)
Check nameStatusExplanation
Title Check✅ PassedThe pull request title clearly summarizes the main change of renaming the optional to mandatory flag in InvestParameters and is concise and specific to the changeset.
✨ 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/rename-investparameter-optional-to-mandatory

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0552d8a and 4eefd29.

📒 Files selected for processing (3)
  • flixopt/elements.py (1 hunks)
  • flixopt/features.py (3 hunks)
  • flixopt/interface.py (6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/interface.py (6)
flixopt/config.py (1)
  • CONFIG (98-153)
flixopt/elements.py (3)
  • transform_data (98-100)
  • transform_data (193-196)
  • transform_data (423-439)
flixopt/components.py (3)
  • transform_data (206-211)
  • transform_data (414-432)
  • transform_data (630-637)
flixopt/effects.py (3)
  • transform_data (173-184)
  • effects (385-386)
  • create_effect_values_dict (292-325)
flixopt/flow_system.py (2)
  • transform_data (343-347)
  • FlowSystem (33-484)
tests/test_storage.py (1)
  • test_investment_parameters (342-390)
flixopt/features.py (3)
flixopt/interface.py (2)
  • minimum_size (867-868)
  • maximum_size (871-872)
flixopt/elements.py (1)
  • label_full (481-482)
flixopt/structure.py (2)
  • label_full (282-283)
  • label_full (389-395)
⏰ 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.12)
  • GitHub Check: test (3.13)
  • GitHub Check: test (3.11)
  • GitHub Check: test (3.10)
🔇 Additional comments (2)
flixopt/elements.py (1)

648-651: Mandatory-aware lower bound looks good

Switching the lower-bound guard to mandatory keeps optional investments free to collapse to zero while still honoring minimum sizing for mandatory cases. No issues spotted here.

flixopt/features.py (1)

49-102: Mandatory gating aligns with the refactor

The refactored branches now consistently hinge on the mandatory flag—fixed-size mandatories stay locked, optional cases keep their binary is_invested, and divest effects only trigger when investment can actually be skipped. Looks solid.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"mode: "warning"instructions: | Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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

@FBumann

Copy link
Copy Markdown
MemberAuthor

#301

@FBumann
FBumann changed the base branch from main to v2.2/mainSeptember 27, 2025 10:39
@FBumann
FBumann merged commit ce7f2bb into v2.2/mainSep 27, 2025
11 checks passed
@coderabbitaicoderabbitaiBot mentioned this pull request Sep 27, 2025
9 tasks
@coderabbitaicoderabbitaiBot mentioned this pull request Oct 6, 2025
9 tasks
@FBumann
FBumann deleted the feature/rename-investparameter-optional-to-mandatory branch October 13, 2025 21:37
@coderabbitaicoderabbitaiBot mentioned this pull request Nov 19, 2025
9 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Rename InvestParameter "optional" to "mandatory"

1 participant

@FBumann