Feature/parameter renames #467
Conversation
* Add truncation to repr of containers * Add truncation to results.py * Changes Made 1. ContainerMixin in structure.py - Changed truncate_repr from bool to int | None - None = show all items (no truncation) - Integer value = maximum number of items to show in repr - Default is None (no truncation) 2. Updated _get_repr() method - Simplified parameters from (truncate: bool, max_items: int) to just (max_items: int | None) - Uses instance's _truncate_repr as default if max_items is not provided - Cleaner logic: truncates only when limit is not None and limit > 0 3. Updated __repr__() method - Now simply calls self._get_repr() without arguments - Respects the instance's truncate_repr setting 4. Simplified _format_grouped_containers() - Removed the conditional logic checking for _truncate_repr - Now just calls repr(container) which automatically respects each container's setting 5. Updated all call sites - flow_system.py: Changed truncate_repr=True → truncate_repr=10 (4 locations) - results.py: Changed truncate_repr=True → truncate_repr=10 (4 locations) - effects.py: Changed truncate_repr: bool = False → truncate_repr: int | None = None and added docstring * Update CHANGELOG.md
* Resample a single concatenated dataarray instead of a Dataset * Performance improvements * Use helper method for resampling speed up resampling * Improve docstring * Improve docstring * avoiding attribute conflicts and empty merge errors * moving method validation earlier for fail-fast behavior * Update CHANGELOG.md * Add new combined select and resample method * Remove code duplication * Add benchmark * Improve becnhmark * Improve becnhmark * Add power user chaining options * Remove becnhmark * Use dask chunking in resample * Make the new methods class methods * Update benchmark_bottleneck.py * Use dataframe based approach * registry pattern * registry pattern * Improve benchmark * Benchmark datarray version * Use dataarray conversion before resampling * Benchmark dask speedup * Add dask chunking * Remove dask chunking due to negligible improvements * Remove benchmark_bottleneck.py * Update CHANGELOG.md * Make ._dataset_... methods self contained, handling time index stuff directly * Use helper method * further deduplication and consistency improvements * improve docstrings * ruff format * fixed the metadata preservation issue in flow_system.py:352-369 * Typo * Add test * Speed up tests * ruff format * Improve tests * Linting fixes * Fix tests
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
WalkthroughThis PR introduces a centralized type system module ( Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant LegacyCode as Legacy Parameter<br/>(e.g., Q_fu, COP)
participant Converter as LinearConverter<br/>Constructor
participant DepHandler as Deprecation<br/>Handler
participant NewField as New Field<br/>(e.g., fuel_flow, cop)
User->>Converter: __init__(..., Q_fu=value, COP=123, **kwargs)
Converter->>DepHandler: _handle_deprecated_kwarg('Q_fu', Q_fu)
DepHandler-->>Converter: mapped_value for fuel_flow
Converter->>DepHandler: _validate_kwargs(**kwargs)
alt kwargs contains legacy names
DepHandler-->>Converter: log deprecation warning
end
Converter->>NewField: Initialize fuel_flow, cop<br/>with mapped/new values
Converter-->>User: Instance created (working)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/interface.py (1)
1-1344: PR description doesn't match the actual changes.The PR title mentions "parameter renames" and the description says "Renaming some parameters in linear_converters.py", but the actual changes in this file (and
modeling.py) constitute a comprehensive type system refactoring across the codebase. The changes involve:
- Introducing new type aliases (
Numeric_TPS,Numeric_PS,Effect_TPS,Effect_PS)- Updating type annotations across multiple public APIs
- Changing default values from
Noneto{}for several parameters- No changes to
linear_converters.pyare visible in the provided filesThis is much more significant than "renaming some parameters" and represents a breaking change to the public API. Please update the PR description to accurately reflect the scope of changes.
🧹 Nitpick comments (11)
flixopt/features.py (2)
151-158: previous_states type hint is broader than what get_previous actually supports*
previous_statesis now annotated asNumeric_TPS | None, but_get_previous_on_durationand_get_previous_off_durationassume anxr.DataArray-like object (they call.iseland rely on array arithmetic). Passing otherNumeric_TPS-compatible types (e.g.np.ndarray,pd.Series,pd.DataFrame) would not satisfy those assumptions.Consider either:
- Narrowing the annotation to
xr.DataArray | None(underTYPE_CHECKING), or- Converting
previous_statesto a DataArray in__init__(e.g. via your existing conversion utilities), so the broaderNumeric_TPStype remains correct.This keeps the public type surface honest about what is actually supported at runtime.
Also applies to: 307-321
515-542: Align ShareAllocationModel Numeric_ hints with supported runtime containers*The constructor now types
total_max/total_minasNumeric_PS | Noneandmax_per_hour/min_per_hourasNumeric_TPS | None, which (perflixopt.types) includes scalars,np.ndarray,pd.Series,pd.DataFrame, andxr.DataArray. In_do_modelingthese are then used directly in arithmetic withnp.infandself._model.hours_per_step(anxr.DataArray) and passed as bounds intoadd_variables.To avoid surprising behavior for callers passing, say,
pd.Series/pd.DataFrame, it may be worth either:
- Narrowing these annotations to the containers you know behave correctly here (e.g.
Scalar | xr.DataArray-like), or- Normalizing them up front (e.g. via your central converter) so
_total_*and_max/_min_per_hourare guaranteed to bexr.DataArrayor scalars when used in the arithmetic.That would keep the new Numeric_* aliases consistent with the actual expectations of this feature model.
Also applies to: 558-570
flixopt/types.py (1)
1-112: Type alias design is coherent; consider clarifying NumericOrBool’s public/internal statusThe
_Numeric/_Bool/_Effectbases and the_TPS/_PS/_Ssuffixed aliases are well structured and align with how other modules use them. The docstring also sets clear expectations about “max dimensions” vs. actual FlowSystem dims.One small polish point:
NumericOrBoolis exported via__all__but the docstring calls it “for internal utilities”. If you intend it to be internal-only, consider omitting it from__all__; if it should be public, rephrase the docstring to avoid implying it’s internal-only.flixopt/effects.py (1)
489-524: Slight type-hint mismatch increate_effect_values_dicthelperThe runtime behavior is fine—
get_effect_labelcorrectly handlesEffectinstances,None(mapping tostandard_effect), and strings—but the signaturedef get_effect_label(eff: Effect | str) -> str:doesn’t includeNoneeven though it’s supported. Consider widening it toEffect | str | None(and, if you care about static checking, reflecting the same in theEffect_TPSkey type) to keep the annotations aligned with the actual behavior.flixopt/linear_converters.py (6)
24-113: Boiler: parameter renames and efficiency encoding are correct, but flows could be validatedThe mapping from
eta,fuel_flow, andthermal_flowintoconversion_factors = [{fuel: eta, thermal: 1}]is consistent with the intendedthermal_flow = fuel_flow × etarelation and withLinearConverterModel’s constraint construction. Deprecation handling forQ_fu/Q_thvia_handle_deprecated_kwargis also in line with the shared pattern and will catch conflicting usage.You might want to add an explicit check that
fuel_flowandthermal_floware notNoneafter deprecation handling so users who forget to pass them get a clearValueErrorinstead of later attribute errors inComponent._connect_flows.
115-209: Power2Heat: renamed flows andetahandling align with existing semantics
etais correctly bounded viacheck_boundsand encoded asconversion_factors = [{power: eta, thermal: 1}], matching the documentedthermal_flow = power_flow × etarelationship. Backward compatibility forP_elandQ_this handled cleanly through_handle_deprecated_kwarg+_validate_kwargs. As withBoiler, a shallow post-init check thatpower_flowandthermal_floware non-Nonewould improve error messages for misconfigured instances.
211-306: HeatPump: COP mapping and deprecation handling are soundThe
copproperty correctly drivesconversion_factors = [{power: cop, thermal: 1}], givingthermal_flow = power_flow × copas intended. The deprecation mapping forCOP,P_el, andQ_this handled via_handle_deprecated_kwarg, and_validate_kwargswill reject unexpected kwargs.Semantically this matches the linear-converter model; no functional issues spotted. Same minor suggestion as above: reject missing
power_flow/thermal_flowearly with a targeted exception instead of relying on later attribute errors.
308-403: CoolingTower: conversion factor sign convention matches the documented relationEncoding
specific_electricity_demandasconversion_factors = [{power: -1, thermal: value}]yields the constraint-power + value×thermal = 0, i.e.power_flow = thermal_flow × specific_electricity_demand, which matches the docstring.specific_electricity_demandis checked withcheck_bounds(..., 0, 1), which is appropriate for a fractional auxiliary demand.Same as for other converters, consider validating that both
power_flowandthermal_floware non-Noneright in__init__for better error messages.
405-533: CHP: dual-efficiency setup and degrees of freedom are encoded correctlyThe two conversion-factor entries
[{fuel: eta_th, thermal: 1}, ...][..., {fuel: eta_el, power: 1}]produce
thermal_flow = fuel_flow × eta_thandpower_flow = fuel_flow × eta_elunderLinearConverterModel, matching the documented relationships. The guards in the setters pluscheck_bounds(eta_th),check_bounds(eta_el), and the aggregate check oneta_th + eta_elare consistent with the “total efficiency ≤ 1” requirement (albeit enforced via warnings for boundary violations).No correctness issues seen; flows are again effectively required in practice, so adding explicit
Nonechecks after deprecation handling would improve robustness and diagnostics.
535-648: HeatPumpWithSource: COP > 1 enforcement and conversion factors look correctThe
copsetter:
- Uses
check_bounds(value, 'cop', ..., 1, 20)to warn about out-of-range values.- Explicitly raises a
ValueErrorwhen any entry is<= 1, ensuring you don’t hit the singularity invalue / (value - 1).- Encodes conversion factors as:
{power: cop, thermal: 1}→thermal_flow = power_flow × cop{heat_source: cop/(cop-1), thermal: 1}→thermal_flow = heat_source_flow × cop/(cop-1), equivalent to the documentedheat_source_flow = thermal_flow × (COP-1)/COP.This aligns with the energy-balance relationships described in the docstring. Again, checking for
Noneflows (power_flow,heat_source_flow,thermal_flow) early would make misconfigurations fail fast with clearer messages.flixopt/elements.py (1)
730-757:absolute_flow_rate_boundstype hint is stricter than its actual return typeThe property is annotated as
tuple[xr.DataArray, xr.DataArray], but for flows with on/off behavior and no mandatory investment the lower bound remains the scalar0(it’s never multiplied by a size), while the upper bound is anxr.DataArray. This is fine for linopy (scalar bounds are broadcast), but it contradicts the annotation and may confuse type checkers.Consider normalizing
lbto a DataArray in those cases, e.g.:- lb = 0 + lb: xr.DataArray | int = 0 if not self.with_on_off: ... - if self.with_investment: - ub = ub_relative * self.element.size.maximum_or_fixed_size - else: - ub = ub_relative * self.element.size - - return lb, ub + if self.with_investment: + ub = ub_relative * self.element.size.maximum_or_fixed_size + else: + ub = ub_relative * self.element.size + + if not isinstance(lb, xr.DataArray): + lb = xr.zeros_like(ub) + return lb, ubThis keeps behavior the same while making the type hint accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
flixopt/__init__.py(1 hunks)flixopt/calculation.py(2 hunks)flixopt/components.py(8 hunks)flixopt/core.py(2 hunks)flixopt/effects.py(4 hunks)flixopt/elements.py(6 hunks)flixopt/features.py(3 hunks)flixopt/flow_system.py(5 hunks)flixopt/interface.py(9 hunks)flixopt/io.py(2 hunks)flixopt/linear_converters.py(29 hunks)flixopt/modeling.py(7 hunks)flixopt/types.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
flixopt/calculation.py (1)
flixopt/core.py (3)
DataConverter(146-554)TimeSeriesData(35-143)drop_constant_arrays(584-614)
flixopt/flow_system.py (2)
flixopt/effects.py (2)
Effect(32-399)EffectCollection(451-633)flixopt/structure.py (1)
weights(224-234)
flixopt/elements.py (2)
flixopt/core.py (1)
PlausibilityError(23-26)flixopt/interface.py (2)
InvestParameters(671-1079)OnOffParameters(1083-1344)
flixopt/components.py (2)
flixopt/core.py (1)
PlausibilityError(23-26)flixopt/interface.py (1)
InvestParameters(671-1079)
flixopt/features.py (2)
flixopt/interface.py (3)
InvestParameters(671-1079)OnOffParameters(1083-1344)Piecewise(87-228)flixopt/elements.py (2)
previous_states(779-792)previous_states(907-924)
flixopt/linear_converters.py (3)
flixopt/core.py (1)
TimeSeriesData(35-143)flixopt/structure.py (4)
_handle_deprecated_kwarg(420-479)_validate_kwargs(481-509)label_full(874-875)label_full(1366-1367)flixopt/elements.py (2)
Flow(296-562)label_full(552-553)
⏰ 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.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
🔇 Additional comments (21)
flixopt/calculation.py (1)
29-29: LGTM: Import cleanup aligns with type system refactoring.The removal of
Scalarfrom imports is consistent with the repository-wide shift to using primitive numeric types (int | float) instead of custom type aliases, as mentioned in the AI summary.flixopt/__init__.py (1)
44-56: Top-level export of dimension-aware types looks goodRe-exporting the Bool_, Effect_, Numeric_* and Scalar aliases at the package level is consistent with the new type system and keeps the runtime surface unchanged.
flixopt/io.py (1)
19-23: numeric_to_str_for_repr annotation correctly aligned with new Numeric_TPS aliasUsing
Numeric_TPSin the TYPE_CHECKING import and as the parameter type matches the function’s actual supported inputs (scalars, ndarray, Series, DataFrame, DataArray); no behavior change, just clearer typing.Also applies to: 655-659
flixopt/features.py (1)
17-21: Updated TYPE_CHECKING imports are consistent with new type systemImporting
FlowSystemDimensions,Numeric_PS, andNumeric_TPSunderTYPE_CHECKINGcleanly supports the annotations in this module without affecting runtime.flixopt/core.py (1)
15-18: DataConverter.to_dataarray correctly updated to use NumericOrBoolImporting
NumericOrBooland using it for thedataparameter brings the signature in line with both the docstring and the isinstance checks (scalars,np.ndarray,pd.Series,pd.DataFrame,xr.DataArray). No behavioral change, just clearer typing and a single source of truth for supported input types.Also applies to: 390-505
flixopt/flow_system.py (2)
25-36: Type imports andweightstyping look consistent with the new type systemImporting
Effect/EffectCollectionand the TYPE_CHECKING-only aliases (Numeric_PS,Numeric_TPS,NumericOrBool, etc.) aligns with how these types are used throughout the file and the rest of the codebase. NarrowingweightstoNumeric_PS | NoneinFlowSystem.__init__matches its actual usage (passed intofit_to_model_coordsand later consumed byFlowSystemModel.weights) without changing behavior.Also applies to: 155-165
523-587:fit_to_model_coords/fit_effects_to_model_coordsremain behaviorally compatibleThe updated signatures (
data: NumericOrBool | None,effect_values: Effect_TPS | Numeric_TPS | None) and the use ofcoordsfiltering bydimskeep the old behavior while making type usage explicit. Delegating effect handling toEffectCollection.create_effect_values_dictis consistent with the newEffect_TPS/Numeric_TPSaliases, and returning{}instead ofNonefor empty mappings is compatible with downstream truthiness checks (if effect.share_from_*:). No functional issues spotted.Also applies to: 588-599
flixopt/effects.py (1)
172-201: Effect bound/share typing and defaults are consistentSwitching the constructor parameters and attributes to
Effect_TPS/Effect_PSandNumeric_PS/Numeric_TPSmatches how these fields are consumed intransform_dataand inEffectCollection.calculate_effect_share_factors. Initializingshare_from_temporal/share_from_periodicto{}whenNonekeeps existing behavior with theif effect.share_from_*:checks while simplifying downstream handling.flixopt/linear_converters.py (1)
651-683:check_boundshelper behaves as a soft guard (warnings only)The updated
check_boundscorrectly:
- Handles
TimeSeriesDataby unwrapping.data.- Supports array-like
Numeric_TPSvalues vianp.all(value > lower_bound)/< upper_bound.- Logs warnings (without raising) when values touch or exceed the exclusive bounds.
This is fine as long as you intend it as a diagnostic tool rather than a hard validator; for parameters that must obey strict constraints (like COP in
HeatPumpWithSource), the additional explicitValueErroryou added is the right way to enforce that.flixopt/elements.py (2)
16-37: PlausibilityError and new numeric/effect types integrate cleanly into FlowImporting
PlausibilityErrorfrom.coreand switching Flow’s constructor signature toNumeric_PS/Numeric_TPS/Effect_TPSmatches the rest of the refactor (InvestParameters, OnOffParameters, FlowSystem)._plausibility_checksraisingPlausibilityErrorfor inconsistent relative bounds is appropriate and keeps error semantics aligned with other modeling plausibility checks. No behavioral issues spotted here.Also applies to: 429-459
779-792:previous_statesreturn type aligns with OnOff usageThe updated annotation
xr.DataArray | Nonematches the actual result fromModelingUtilitiesAbstract.to_binaryand howprevious_statesis consumed inOnOffModel. The scalar/1D handling ofprevious_flow_rateis unchanged; no issues detected.flixopt/components.py (3)
31-37: LinearConverter:conversion_factorstyping and transformation are coherentTyping
conversion_factorsaslist[dict[str, Numeric_TPS]] | Nonematches how_transform_conversion_factorsfeeds values intofit_to_model_coords, and the method correctly returns a list ofdict[str, xr.DataArray]used byLinearConverterModel. The PlausibilityError checks (degrees_of_freedom, flows present inself.flows) remain valid under the new types.Also applies to: 167-237
383-433: Storage: new numeric type aliases and charge-state bounds stay consistentSwitching
capacity_in_flow_hours, relative charge-state parameters, efficiencies, and losses toNumeric_PS/Numeric_TPSwhile transforming everything throughfit_to_model_coordskeeps the existing behavior and plays nicely withInvestParameters._absolute_charge_state_boundsnow explicitly returnstuple[xr.DataArray, xr.DataArray]by multiplying relative bounds with either scalar/DataArray capacity or InvestParameters’ min/max, which matches howStorageModeluses these bounds when creatingcharge_statevariables.Also applies to: 438-481, 919-961
547-573: Transmission: loss parameters now aligned with the shared numeric typesTyping
relative_lossesandabsolute_lossesasNumeric_TPS | Noneand converting them viafit_to_model_coordsintransform_datais consistent with the rest of the modeling layer.create_transmission_equationstill encodes
out = in × (1 - relative_losses)- plus
on * absolute_losseswhen present,which matches the documented behavior. The guard in
TransmissionModel.__init__that auto-attachesOnOffParameterswhenever non-zeroabsolute_lossesare present is still valid under the new types.Also applies to: 660-773
flixopt/modeling.py (2)
205-569: Consistent type system refactoring looks good.The systematic replacement of
TemporalDatawithxr.DataArraythroughout the module is consistent and aligns with the broader type system changes described in the PR summary. The changes maintain parameter names and logic while only updating type annotations.
120-142: No backward compatibility issue found. TemporalData does not exist in the codebase.The review comment references a type change from
TemporalDatatoxr.DataArray, butTemporalDatadoes not appear anywhere in the codebase. The actual parameter acceptsNumeric_TPS | None, which is a type alias that already includesxr.DataArrayin its union of accepted types. All existing call sites pass eitherxr.DataArrayobjects or values derived from arithmetic operations onxr.DataArray, making them fully compatible with the annotated type. The type annotation change is correct and poses no backward compatibility concerns.Likely an incorrect or invalid review comment.
flixopt/interface.py (5)
1001-1047: Deprecated property return types correctly updated.The return type annotations for deprecated properties have been updated to align with the new type system. This maintains consistency between the deprecated interface and the new attribute types.
19-23: All type aliases are properly defined and exported.Verification confirms that
flixopt/types.pyexists, contains all four type aliases (Effect_PS,Effect_TPS,Numeric_PS,Numeric_TPS) as properly structuredTypeAliasdefinitions, and exports them in__all__. The base types (_Numeric,_Effect) are well-defined, and the module includes clear documentation on dimension semantics. The imports ininterface.pyare correct and complete.
1264-1286: No backward compatibility issues found.Verification confirms the default value changes are safe:
Truthiness checks: Code uses
if self.parameters.effects_per_running_hour:(features.py lines 251, 261). Since{}andNoneare both falsy in Python, behavior is identical.Dictionary operations: The
.items()calls on these attributes (features.py lines 256, 265) work correctly with{}and produce the same empty iterator result as a None case would.No explicit None checks: Searching the codebase found no patterns like
is Noneor== Noneon these attributes, confirming the code treats them as dictionaries from the start.The change from
Noneto{}defaults is semantically equivalent and maintains full backward compatibility.
874-924: Review comment is incorrect. No breaking change to None handling.The codebase search found no production code that checks for
Noneon these effect parameters—only one test validates the new empty-dict default is correct. The function signature still acceptsNoneas input; the internal conversion fromNoneto{}is transparent to callers and cannot break downstream code. Test suite confirms this behavior is intentional.Likely an incorrect or invalid review comment.
1-1344: Testing verification cannot be completed in sandbox environment.The sandbox lacks necessary dependencies (linopy, mypy) and git history to verify the testing request. Additionally:
- No test files related to
interface.pywere located in the repository- The repository is not a git repository in this environment, so the actual PR diff cannot be retrieved
- The provided code snippet shows only the final state, not what changed
To properly verify this review comment, please manually confirm:
What changed in this PR? The review assumes "type system changes" but only the final code state is visible. Provide the git diff or describe specific breaking changes introduced.
Do existing tests pass? Can you run
pytest tests/ -vlocally and confirm all tests pass with these changes?Are there type errors? Run
mypy flixopt/interface.py --strictlocally to confirm type checking passes.PR objectives status? The review references unchecked testing boxes—verify whether testing was actually performed before this review.
Without this information, I cannot confirm whether the testing request addresses real issues or is a procedural precaution.
|
|
||
| @property | ||
| def main_results(self) -> dict[str, Scalar | dict]: | ||
| def main_results(self) -> dict[str, int | float | dict]: |
There was a problem hiding this comment.
Incomplete return type annotation: missing list type.
The return type annotation dict[str, int | float | dict] doesn't account for the 'Buses with excess' key at lines 134-146, which returns a list of dicts. Type checkers will flag this inconsistency.
Apply this diff to fix the type annotation:
- def main_results(self) -> dict[str, int | float | dict]:
+ def main_results(self) -> dict[str, int | float | dict | list]:🤖 Prompt for AI Agents
In flixopt/calculation.py around line 106, the return type annotation def
main_results(self) -> dict[str, int | float | dict] is missing the case where a
value is a list of dicts (the 'Buses with excess' key); update the annotation to
include list[dict[str, int | float | dict]] (e.g. def main_results(self) ->
dict[str, int | float | dict | list[dict[str, int | float | dict]]]) so type
checkers accept the list-of-dicts return value.
FBumann
changed the base branch from
main
to
feature/simplify-data-converter
|
Continue in #468 |
Description
Renaming some parameters in
linear_converters.pyType of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Refactor
Q_fu→fuel_flow,Q_th→thermal_flow,P_el→power_flow) with backward compatibility maintained.Documentation