Rename multiple parameters #362
Conversation
* Change .optional to .mandatory * Change .optional to .mandatory * Remove not needed properties
WalkthroughReplace invest/operation domain names with nontemporal/temporal; switch InvestParameters from an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as InvestParameters
participant Effect as Effect / Feature
participant Model as FlowModel
User->>API: __init__(..., mandatory, optional=None)
Note right of API #lightgrey: if old `optional` provided → emit DeprecationWarning\nstore `mandatory = not optional`
API-->>User: InvestParameters instance
User->>Effect: create Effect(minimum_temporal, minimum_nontemporal, ...)
Effect->>Effect: map deprecated kwargs → temporal/nontemporal (warn)
Effect-->>User: Effect instance wired to models
User->>Model: do_modeling(element with InvestParameters)
alt element.size is InvestParameters
Model->>Model: if not size.mandatory → allow zero lower bound, create `is_invested` and divest shares
else size.mandatory → enforce positive lower bound (no optional-invest path)
else non-invest size
Model->>Model: use existing bounds
end
Model->>Model: expose variables as `... (temporal)`, `... (nontemporal)`, and flattened total key `...`
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests
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 |
* Rename effect domains * Rename effect domains * Ensure backwards compatability * Improve * Improve * Bugfix IO with deprectaed params * Add guards for extra kwargs * Add guards for extra kwargs * centralize logic for deprectaed params * Move handlign from centralized back to classes in a dedicated method * Improce property handling * Move handling to Interface class * Getting lost * Revert "Getting lost" This reverts commit 3c0db76. * Revert "Move handling to Interface class" This reverts commit 09bdeec. * Revert "Improce property handling" This reverts commit 5fe2c64. * Revert "Move handlign from centralized back to classes in a dedicated method" This reverts commit 9f4c1f6. * Revert "centralize logic for deprectaed params" This reverts commit 4a82574. * Add "" to warnings * Revert change in examples * Improve BackwardsCompatibleDataset * Add unit tests for backwards compatability * Remove backwards compatible dataset * Renamed maximum_temporal_per_hour to maximum_per_hour and minimum_temporal_per_hour to minimum_per_hour * Add entires to CHANGELOG.md * Remove backwards compatible dataset * Remove unused imports
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
examples/03_Calculation_types/example_calculation_types.py (2)
213-218: Make the total-costs bar chart use calc names on the x‑axis (cleaner UX).Feed a (n_calcs x 1) DataFrame so bars are labeled by calculation type and set an explicit x‑label.
Apply this diff:
- fx.plotting.with_plotly( - pd.DataFrame(get_solutions(calculations, 'costs(temporal)|per_timestep').to_dataframe().sum()).T, - mode='bar', - title='Total Cost Comparison', - ylabel='Costs [€]', - ).update_layout(barmode='group').write_html('results/Total Costs.html') + fx.plotting.with_plotly( + get_solutions(calculations, 'costs(temporal)|per_timestep').to_dataframe().sum().rename('Total costs').to_frame(), + mode='bar', + title='Total Cost Comparison', + ylabel='Costs [€]', + xlabel='Calculation type', + ).write_html('results/Total Costs.html')
206-212: Align plot title and output filename with ‘Temporal’ terminology- title='Operation Cost Comparison', + title='Temporal Cost Comparison', ylabel='Costs [€]', - ).write_html('results/Operation Costs.html') + ).write_html('results/Temporal Costs.html')No stale
costs(operation)or legacy|total_per_timestepreferences detected.flixopt/structure.py (1)
205-234: Generalize kwargs validation and drop redundant import.Handle any VAR_KEYWORD name (not just "kwargs") and avoid the inner import.
Apply:
- import inspect - - sig = inspect.signature(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'} + sig = inspect.signature(self.__init__) + # allow for any VAR_KEYWORD name; only treat explicit parameters as known + known_params = { + name + for name, p in sig.parameters.items() + if p.kind != inspect.Parameter.VAR_KEYWORD and name != 'self' + } + extra_kwargs = {k: v for k, v in kwargs.items() if k not in known_params}flixopt/features.py (1)
959-965: ShareAllocationModel totals built via zero-equality is fine; minor clarity nitBuilding totals by initializing
== 0and subtracting shares is acceptable. Consider a brief comment explaining the pattern to ease future maintenance.Also applies to: 976-984
flixopt/effects.py (1)
266-386: Consider removing or setting a deprecation timeline for these compatibility properties.While these property shims ensure backwards compatibility, maintaining 12 properties (6 getters + 6 setters) adds significant boilerplate. Each property access triggers a deprecation warning, which is good, but consider:
- Adding a version/date when these will be removed (e.g., "Will be removed in v3.0")
- Using a decorator pattern to reduce code duplication
Here's a decorator-based approach to reduce duplication:
def deprecated_property(old_name: str, new_name: str): """Create a deprecated property that proxies to a new attribute.""" def make_property(): def getter(self): warnings.warn( f"Property '{old_name}' is deprecated. Use '{new_name}' instead.", DeprecationWarning, stacklevel=2, ) return getattr(self, new_name) def setter(self, value): warnings.warn( f"Property '{old_name}' is deprecated. Use '{new_name}' instead.", DeprecationWarning, stacklevel=2, ) setattr(self, new_name, value) return property(getter, setter, doc=f"DEPRECATED: Use '{new_name}' property instead.") return make_property() # Then in the class: minimum_operation = deprecated_property('minimum_operation', 'minimum_temporal') maximum_operation = deprecated_property('maximum_operation', 'maximum_temporal') # ... etc
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
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(2 hunks)flixopt/features.py(8 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 (11)
tests/test_io.py (1)
flixopt/structure.py (1)
solution(76-94)
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)
tests/test_linear_converter.py (3)
flixopt/structure.py (2)
constraints(462-463)variables(458-459)flixopt/results.py (4)
constraints(229-233)constraints(379-387)variables(222-226)variables(368-376)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/structure.py (3)
constraints(462-463)variables(458-459)hours_per_step(97-98)flixopt/results.py (4)
constraints(229-233)constraints(379-387)variables(222-226)variables(368-376)tests/conftest.py (1)
assert_conequal(455-471)
flixopt/features.py (2)
flixopt/structure.py (3)
label_full(312-313)label_full(419-425)add(365-389)flixopt/effects.py (2)
effects(599-600)add_share_to_effects(637-649)
flixopt/calculation.py (1)
flixopt/structure.py (1)
solution(76-94)
tests/test_functional.py (3)
flixopt/structure.py (1)
variables(458-459)flixopt/results.py (2)
variables(222-226)variables(368-376)tests/test_bus.py (1)
TestBusModel(6-65)
tests/test_integration.py (1)
flixopt/results.py (1)
solution_without_overlap(859-872)
tests/test_effect.py (3)
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/core.py (1)
timesteps(929-931)
flixopt/effects.py (3)
flixopt/structure.py (5)
_validate_kwargs(205-233)transform_data(118-120)label_full(312-313)label_full(419-425)add(365-389)flixopt/flow_system.py (2)
transform_data(343-347)create_time_series(349-373)flixopt/features.py (1)
ShareAllocationModel(923-1024)
⏰ 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.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
🔇 Additional comments (41)
flixopt/components.py (4)
973-1003: Deprecation messages read clearly.Quoted arg names improve clarity for users reading warnings. No issues.
1004-1006: Good: validate and fail fast on stray kwargs.This prevents silent misconfiguration after legacy remapping.
Please confirm no downstream code relied on passing extra kwargs here to be forwarded to super().init.
1128-1141: Source: deprecation handling + validation LGTM.Warning text and leftover kwargs validation are appropriate.
1259-1272: Sink: deprecation handling + validation LGTM.Consistent with Source/SourceAndSink behavior.
tests/conftest.py (1)
65-66: Test updated to maximum_per_hour LGTM.Matches the new per-hour naming.
tests/test_linear_converter.py (2)
187-192: Update to Costs(temporal) LGTM.Aligns with temporal/nontemporal terminology.
491-496: Consistent temporal renaming in assertions LGTM.Matches the new constraint/variable keys.
CHANGELOG.md (1)
51-57: Verify deprecation shims & test coverage
- Ensure effects/interface defines deprecation aliases for each old parameter→new parameter mapping.
- Ensure tests import or reference every old alias (minimum_investment, maximum_investment, minimum_operation, maximum_operation, minimum_operation_per_hour, maximum_operation_per_hour) to guarantee backwards compatibility.
tests/test_functional.py (3)
115-115: LGTM! Effect result key updated correctly.The change from
costs|totaltocostsaligns with the broader PR objective to simplify effect result key structure. The search results confirm flixopt is an energy optimization framework with temporal/nontemporal parameters, and this flattened key structure improves clarity.
125-129: LGTM! Temporal effect key updated correctly.The change from
costs(operation)|total_per_timesteptocosts(temporal)|per_timestepproperly implements the operation → temporal naming migration mentioned in the AI summary. This makes the temporal nature of these costs more explicit.
132-136: LGTM! Component-specific temporal effect key updated correctly.The change from
Gastarif(Gas)->costs(operation)toGastarif(Gas)->costs(temporal)maintains consistency with the temporal naming convention across component-specific effect references.flixopt/calculation.py (1)
83-84: LGTM! Effect totals updated to use temporal/nontemporal naming.The changes from
effect.model.operation.total.solution.valuesandeffect.model.invest.total.solution.valuestoeffect.model.temporal.total.solution.valuesandeffect.model.nontemporal.total.solution.valuesproperly implement the operation → temporal and invest → nontemporal parameter renaming as described in the AI summary.tests/test_io.py (1)
45-46: LGTM! Solution dictionary key updated correctly.The change from
calculation_0.results.solution['costs|total']tocalculation_0.results.solution['costs']aligns with the flattened key structure mentioned in the AI summary, removing the nested composite keys in favor of simpler direct keys.tests/test_integration.py (6)
182-185: LGTM! Result key updated for consistency.The change from
costs|totaltocostsmaintains consistency with the flattened key structure being implemented across all tests.
188-201: LGTM! Temporal per-timestep costs key updated correctly.The change from
costs(operation)|total_per_timesteptocosts(temporal)|per_timestepproperly reflects the operation → temporal naming migration while also simplifying the key structure.
204-232: LGTM! All component temporal cost references updated correctly.All changes from
*->costs(operation)to*->costs(temporal)consistently implement the operation → temporal naming convention across different components (CO2, Kessel, Gastarif, Einspeisung, KWK).
235-244: LGTM! Nontemporal cost references updated correctly.The changes from
*->costs(invest)to*->costs(nontemporal)properly implement the invest → nontemporal naming migration for investment-related costs.
247-255: LGTM! Effect result keys updated for temporal/nontemporal split.The changes from
CO2|totaltoCO2(temporal)and addition ofCO2(nontemporal)properly separate temporal and nontemporal CO2 effects, consistent with the new naming convention.
410-413: LGTM! Modeling calculation cost result keys updated consistently.Both full/aggregated and segmented calculation result assertions have been updated to use the new key structure (
costsinstead ofcosts|total, andcosts(temporal)|per_timestepfor per-timestep data).Also applies to: 416-419
flixopt/elements.py (2)
648-649: LGTM! Investment parameter logic updated to use mandatory flag.The change from
if self.element.size.optionaltoif not self.element.size.mandatorycorrectly implements the new mandatory vs optional semantics for InvestParameters. The logic inversion maintains the same behavior while using the new parameter structure.
578-578: LGTM! Share target updated to temporal.The change from
share_target='operation'toshare_target='temporal'properly implements the operation → temporal naming migration for effect share targets.tests/test_flow.py (5)
102-115: Temporal effects wiring for per-flow-hour looks correctNames and expressions align with the new temporal category; constraints tie to hours_per_step as expected.
404-419: Investment effects correctly moved to nontemporalPresence checks and constraint formulations for fixed and specific effects are consistent with the new naming.
440-446: Divest effects formulation is correctConstraint equivalence matches the added share expression (-is_invested * factor + factor).
543-556: Temporal running-hour effects are correctly expressedPer-timestep accumulation via on * hours_per_step is correct under the temporal target.
889-895: Startup cost effect moved to temporal is consistentConstraint ties startup count to temporal effect per timestep as intended.
flixopt/features.py (4)
66-73: Optional-invest gating via not mandatory is sound
is_investedcreation and bounds are only added when investment is not mandatory. Looks good.
83-109: Effects re-targeted to nontemporal are coherent
- Fixed effects: gated by
is_investedwhen present.- Divest effects: only for non-mandatory, with correct expression.
- Specific effects: proportional to size.
All correctly routed to 'nontemporal'.
739-751: On/Off effects correctly routed to temporalBoth running-hour and switch-on effects feed into temporal shares with appropriate expressions.
49-66: Backward compatibility preserved:optionalstill supported via deprecation shim
InvestParameters.__init__mapsoptionaltomandatory(with a DeprecationWarning), so all existingoptional=call sites behave unchanged.tests/test_effect.py (6)
17-34: Variable set aligns with new effect structurePresence and shapes of Effect1, temporal, nontemporal, and per_timestep variables are correct.
36-49: Aggregation constraints match new semantics
Effect1 == temporal + nontemporal, temporal sum over per_timestep, and zero defaults are correct.
70-81: Bounds mapping to temporal/nontemporal/per_hour is correctLower/upper checks follow the new parameter names and expected scaling by hours_per_step.
83-106: Bound-related constraints are consistentAggregate and per-timestep equalities match the model's ShareAllocation setup.
122-137: Cross-effect share variables use the correct new namingNames like
Effect1(nontemporal)->Effect2(nontemporal)and temporal counterparts are consistent.
139-160: Cross-effect constraint wiring is correctNontemporal shares scale totals; temporal shares scale per_timestep. Looks good.
flixopt/effects.py (5)
176-252: LGTM! Comprehensive deprecation handling with clear error messages.The deprecation logic properly handles all six deprecated parameters with:
- Clear deprecation warnings indicating the new parameter names
- Mutual exclusion checks preventing both old and new parameters from being specified
- Consistent pattern across all parameter migrations
415-443: LGTM! Clean refactoring of domain terminology.The renaming from
invest/operationtonontemporal/temporalis consistently applied throughout the model initialization. The label formatting with parentheses (e.g.,(temporal),(nontemporal)) provides clear distinction in the model structure.
641-649: Type annotation limits target to valid values.Good use of
Literal['temporal', 'nontemporal']to enforce valid target values at the type level, preventing invalid targets at development time.
669-683: Missing tests foradd_shareontemporalandnontemporal
I see tests forspecific_share_to_other_effects_operationand…_investbut none asserting calls totemporal.add_shareornontemporal.add_share. Add or update tests to cover both share paths.
161-161:**kwargsis validated to prevent silent typos
_validate_kwargsis called after popping only the explicitly deprecated keys (with warnings), so any misspelled or unexpected parameters will raise a TypeError rather than be silently ignored.
Change .optional to .mandatory
Change .optional to .mandatory
Remove not needed properties
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
#301 #360
Testing
Checklist
Summary by CodeRabbit
New Features
Refactor
Chores