Fix/small fixes#346
Conversation
|
Warning Rate limit exceeded@FBumann has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (3)
WalkthroughUpdates COP getter/setter logic for HeatPump and HeatPumpWithSource, fixes ShareAllocationModel total_max initialization, corrects an error message, enables configurable NetCDF compression level, and documents three fixes in CHANGELOG. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Code
participant IO as io.save_dataset_to_netcdf
participant DS as xarray.Dataset
participant FS as Filesystem
U->>IO: save_dataset_to_netcdf(ds, path, compression=k, apply_encoding=True)
IO->>DS: build per-variable encoding {zlib: true, complevel: k}
note right of DS #D3E4CD: complevel uses provided k (not fixed 5)
IO->>FS: write NetCDF with encoding
FS-->>U: file persisted
sequenceDiagram
autonumber
participant U as Caller
participant HP as HeatPump / HeatPumpWithSource
participant C1 as Conversion 1
participant C2 as Conversion 2
U->>HP: set COP(value)
HP->>C1: set P_el = COP, set Q_th = 1
HP->>C2: set Q_ab = Q_th*(COP-1)/COP -- with Q_th=1
HP-->>U: internal conversions updated (COP tied to P_el)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 (2)
flixopt/io.py (1)
219-238: Bug: path may be str; .suffix will crash.save_dataset_to_netcdf accepts Union[str, Path] but uses path.suffix directly. Coerce to Path first.
def save_dataset_to_netcdf( ds: xr.Dataset, path: Union[str, pathlib.Path], compression: int = 0, ) -> None: @@ - if path.suffix not in ['.nc', '.nc4']: + # Normalize to Path + path = pathlib.Path(path) if not isinstance(path, pathlib.Path) else path + if path.suffix not in ['.nc', '.nc4']: raise ValueError(f'Invalid file extension for path {path}. Only .nc and .nc4 are supported') @@ - else {data_var: {'zlib': True, 'complevel': compression} for data_var in ds.data_vars}, + else {data_var: {'zlib': True, 'complevel': int(compression)} for data_var in ds.data_vars},flixopt/linear_converters.py (1)
547-568: Validate COP before building conversion_factors in init.The constructor computes COP/(COP-1) before any checks; if COP==1 (or contains 1), this will explode. Validate first.
def __init__( self, label: str, COP: NumericDataTS, P_el: Flow, Q_ab: Flow, Q_th: Flow, on_off_parameters: OnOffParameters = None, meta_data: Optional[Dict] = None, ): - # super: + # Validate COP strictly > 1 to avoid division by zero + check_bounds(COP, 'COP', label, 1, 20) + _arr = COP.data if isinstance(COP, TimeSeriesData) else COP + if np.any(np.asarray(_arr) <= 1): + raise ValueError(f'HeatPumpWithSource({label}).COP must be strictly > 1.') + # Build equations electricity = {P_el.label: COP, Q_th.label: 1} heat_source = {Q_ab.label: COP / (COP - 1), Q_th.label: 1}
🧹 Nitpick comments (3)
CHANGELOG.md (1)
48-50: Polish wording and formatting for consistency.Add backticks and periods; clarify the last bullet.
- - Fix custom compression levels in io.save_dataset_to_netcdf - - Fix `total_max` did not work when total min was not used. +- Fix custom compression levels in `io.save_dataset_to_netcdf`. +- Fix `total_max` not applied when `total_min` was not used.flixopt/io.py (1)
222-231: Validate compression range (0–9).Prevent invalid values early instead of relying on downstream errors.
- apply_encoding = False + if not (0 <= int(compression) <= 9): + raise ValueError(f'Invalid compression={compression}. Must be an integer in [0, 9].') + apply_encoding = Falseflixopt/features.py (1)
593-597: Docstring mismatch: raises ValueError, not TypeError.Align the “Raises” section with the implementation.
- Raises - ------ - TypeError - If the length of binary_values and dt_in_hours is not equal, but None is a scalar. + Raises + ------ + ValueError + If the last consecutive duration length exceeds the provided hours_per_timestep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)flixopt/features.py(2 hunks)flixopt/io.py(1 hunks)flixopt/linear_converters.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
flixopt/linear_converters.py (2)
flixopt/structure.py (3)
label(381-382)label_full(278-279)label_full(385-391)flixopt/elements.py (1)
label_full(471-472)
⏰ 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.10)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
🔇 Additional comments (3)
flixopt/features.py (2)
616-621: LGTM: clearer error detail.Message now reports the actual consecutive-on length; good improvement.
946-952: Defaulting to ±∞ is fine; confirm solver tolerance.Switching None→±∞ means bounds are always applied (often as ±∞). Confirm linopy/solver handles infinite bounds without numeric side effects in this context.
Would you run a small scenario ensuring total_per_timestep lower/upper accept ±∞ and don’t emit solver warnings?
flixopt/linear_converters.py (1)
575-576: LGTM: COP getter now tied to P_el.Getter correctly reflects the new mapping.
| @property | ||
| def COP(self): # noqa: N802 | ||
| return self.conversion_factors[0][self.Q_th.label] | ||
| return self.conversion_factors[0][self.P_el.label] | ||
|
|
||
| @COP.setter | ||
| def COP(self, value): # noqa: N802 | ||
| check_bounds(value, 'COP', self.label_full, 1, 20) | ||
| self.conversion_factors[0][self.Q_th.label] = value | ||
| self.conversion_factors[1][self.Q_th.label] = value / (value - 1) | ||
| # electricity equation: COP * P_el == 1 * Q_th | ||
| self.conversion_factors[0][self.P_el.label] = value | ||
| self.conversion_factors[0][self.Q_th.label] = 1 | ||
| # heat source equation: (COP/(COP-1)) * Q_ab == 1 * Q_th -> Q_ab = Q_th*(COP-1)/COP | ||
| self.conversion_factors[1][self.Q_ab.label] = value / (value - 1) | ||
| self.conversion_factors[1][self.Q_th.label] = 1 | ||
|
|
There was a problem hiding this comment.
Guard against COP ≤ 1 to avoid division by zero/infinity.
Setter computes value/(value-1). With COP==1 this blows up; check_bounds only warns. Enforce strictly >1.
@COP.setter
def COP(self, value): # noqa: N802
- check_bounds(value, 'COP', self.label_full, 1, 20)
- # electricity equation: COP * P_el == 1 * Q_th
- self.conversion_factors[0][self.P_el.label] = value
- self.conversion_factors[0][self.Q_th.label] = 1
- # heat source equation: (COP/(COP-1)) * Q_ab == 1 * Q_th -> Q_ab = Q_th*(COP-1)/COP
- self.conversion_factors[1][self.Q_ab.label] = value / (value - 1)
- self.conversion_factors[1][self.Q_th.label] = 1
+ check_bounds(value, 'COP', self.label_full, 1, 20)
+ # Strict validation: avoid singularity at COP <= 1
+ arr = value.data if isinstance(value, TimeSeriesData) else value
+ if np.any(np.asarray(arr) <= 1):
+ raise ValueError(f'{self.label_full}.COP must be strictly > 1 for HeatPumpWithSource.')
+ # electricity equation: COP * P_el == 1 * Q_th
+ self.conversion_factors[0][self.P_el.label] = value
+ self.conversion_factors[0][self.Q_th.label] = 1
+ # heat source equation: (COP/(COP-1)) * Q_ab == 1 * Q_th -> Q_ab = Q_th*(COP-1)/COP
+ self.conversion_factors[1][self.Q_ab.label] = value / (value - 1)
+ self.conversion_factors[1][self.Q_th.label] = 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property | |
| def COP(self): # noqa: N802 | |
| return self.conversion_factors[0][self.Q_th.label] | |
| return self.conversion_factors[0][self.P_el.label] | |
| @COP.setter | |
| def COP(self, value): # noqa: N802 | |
| check_bounds(value, 'COP', self.label_full, 1, 20) | |
| self.conversion_factors[0][self.Q_th.label] = value | |
| self.conversion_factors[1][self.Q_th.label] = value / (value - 1) | |
| # electricity equation: COP * P_el == 1 * Q_th | |
| self.conversion_factors[0][self.P_el.label] = value | |
| self.conversion_factors[0][self.Q_th.label] = 1 | |
| # heat source equation: (COP/(COP-1)) * Q_ab == 1 * Q_th -> Q_ab = Q_th*(COP-1)/COP | |
| self.conversion_factors[1][self.Q_ab.label] = value / (value - 1) | |
| self.conversion_factors[1][self.Q_th.label] = 1 | |
| @property | |
| def COP(self): # noqa: N802 | |
| return self.conversion_factors[0][self.P_el.label] | |
| @COP.setter | |
| def COP(self, value): # noqa: N802 | |
| check_bounds(value, 'COP', self.label_full, 1, 20) | |
| # Strict validation: avoid singularity at COP <= 1 | |
| arr = value.data if isinstance(value, TimeSeriesData) else value | |
| if np.any(np.asarray(arr) <= 1): | |
| raise ValueError(f'{self.label_full}.COP must be strictly > 1 for HeatPumpWithSource.') | |
| # electricity equation: COP * P_el == 1 * Q_th | |
| self.conversion_factors[0][self.P_el.label] = value | |
| self.conversion_factors[0][self.Q_th.label] = 1 | |
| # heat source equation: (COP/(COP-1)) * Q_ab == 1 * Q_th -> Q_ab = Q_th*(COP-1)/COP | |
| self.conversion_factors[1][self.Q_ab.label] = value / (value - 1) | |
| self.conversion_factors[1][self.Q_th.label] = 1 |
🤖 Prompt for AI Agents
In flixopt/linear_converters.py around lines 573-586, the COP setter can perform
value/(value-1) which will divide by zero or produce infinite values when COP ==
1; enforce COP > 1 by adding an explicit guard before doing the division: if
value <= 1 raise a ValueError (or use the project's preferred exception) with a
clear message (e.g. "COP must be > 1"), then proceed to set conversion_factors
as before; do not rely solely on check_bounds which currently allows 1.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_on_hours_computation.py (3)
7-9: Rename test class/docstring to reflect the actual API name.They target compute_consecutive_hours_in_state, not “consecutive duration”.
-class TestComputeConsecutiveDuration: - """Tests for the compute_consecutive_duration static method.""" +class TestComputeConsecutiveHoursInState: + """Tests for compute_consecutive_hours_in_state."""
45-46: Optionally assert error message content for stronger guarantees.If the implementation now includes details (e.g., lengths), add a regex match.
- with pytest.raises(ValueError): + with pytest.raises(ValueError, match=r"(length|shape|incompatible)"): ConsecutiveStateModel.compute_consecutive_hours_in_state(binary_values, hours_per_timestep)
70-73: Fix misleading comment (“2D arrays” are not used).Minor clarity tweak.
- # Case 6: Mix of None, 1D and 2D arrays + # Case 6: Mix of None and 1D arrays
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_on_hours_computation.py(1 hunks)
⏰ 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.10)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
🔇 Additional comments (1)
tests/test_on_hours_computation.py (1)
45-46: Approve: ValueError change is correct.compute_consecutive_hours_in_state (flixopt/features.py) raises ValueError (length check); tests/test_on_hours_computation.py now expects ValueError and I found no tests still asserting TypeError for this API.
1 participant
Description
Apply multiple small fixes
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Bug Fixes
Documentation