refactor: make Comparison follow dict/Mapping protocol#671
Conversation
Iteration now yields case names instead of (name, FlowSystem) pairs, matching Python's dict/Mapping convention. Added keys(), values(), and items() methods for explicit access. Users iterating pairs should switch to `for name, fs in comp.items():`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 35 seconds. ⌛ 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. 📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (2)
tests/test_comparison.py (1)
230-233: Nit: docstring mentions "without warning" but the test doesn't assert it.The docstring says
items()returns pairs "without warning", but the test body only compares equality. If the no-warning behavior is part of the contract (vs. the prior implementation that may have emitted one), wrap the call inwarnings.catch_warnings()withsimplefilter('error'), or drop the "without warnings" phrase from the docstring to avoid misleading future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_comparison.py` around lines 230 - 233, The test docstring claims items() returns pairs "without warning" but the body doesn't assert that no warnings occur; update the test_items function to enforce the no-warning contract by calling comp.items() inside a warnings.catch_warnings() context and set warnings.simplefilter('error') so any warning would fail the test (refer to test_items, comp = fx.Comparison([...]), and the items() call on comp with optimized_base and optimized_with_chp), or alternatively remove the "without warning" phrase from the docstring if the no-warning behavior is not required.flixopt/comparison.py (1)
239-254: Minor:keys()/values()/items()allocate a throwaway dict on every call.Each invocation goes through
self.flow_systems, which rebuilds a brand-new dict viadict(zip(self._names, self._systems, strict=True)). The returned view is only valid because that temporary dict is held alive by the view object itself — functionally fine, but you pay an O(n) dict construction for what should be cheap accessors, and successive calls return views backed by different dict instances (so identity comparisons between them won't hold).Since
_namesand_systemsare already the source of truth, you can avoid the allocation entirely — either by caching the dict or by returning lightweight iterables. If strictKeysView/ValuesView/ItemsViewtyping is important, cache on first access:Proposed refactor
- `@property` - def flow_systems(self) -> dict[str, FlowSystem]: - """Access underlying FlowSystems as a dict mapping name → FlowSystem.""" - return dict(zip(self._names, self._systems, strict=True)) + `@property` + def flow_systems(self) -> dict[str, FlowSystem]: + """Access underlying FlowSystems as a dict mapping name → FlowSystem.""" + if self._flow_systems_cache is None: + self._flow_systems_cache = dict(zip(self._names, self._systems, strict=True)) + return self._flow_systems_cache(with a corresponding
self._flow_systems_cache: dict[str, FlowSystem] | None = Nonein__init__).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flixopt/comparison.py` around lines 239 - 254, The keys()/values()/items() accessors currently call the flow_systems property which recreates a dict each time via dict(zip(self._names, self._systems, strict=True)), causing O(n) allocation on every call; change flow_systems to use a cached mapping (add self._flow_systems_cache: dict[str, FlowSystem] | None = None in __init__) and have flow_systems return the cached dict if present or build-and-store it once, and ensure any mutating code that updates _names or _systems invalidates the cache (set _flow_systems_cache = None) so the cached dict stays correct; this preserves KeysView/ValuesView/ItemsView semantics while avoiding repeated allocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@flixopt/comparison.py`:
- Around line 239-254: The keys()/values()/items() accessors currently call the
flow_systems property which recreates a dict each time via dict(zip(self._names,
self._systems, strict=True)), causing O(n) allocation on every call; change
flow_systems to use a cached mapping (add self._flow_systems_cache: dict[str,
FlowSystem] | None = None in __init__) and have flow_systems return the cached
dict if present or build-and-store it once, and ensure any mutating code that
updates _names or _systems invalidates the cache (set _flow_systems_cache =
None) so the cached dict stays correct; this preserves
KeysView/ValuesView/ItemsView semantics while avoiding repeated allocations.
In `@tests/test_comparison.py`:
- Around line 230-233: The test docstring claims items() returns pairs "without
warning" but the body doesn't assert that no warnings occur; update the
test_items function to enforce the no-warning contract by calling comp.items()
inside a warnings.catch_warnings() context and set
warnings.simplefilter('error') so any warning would fail the test (refer to
test_items, comp = fx.Comparison([...]), and the items() call on comp with
optimized_base and optimized_with_chp), or alternatively remove the "without
warning" phrase from the docstring if the no-warning behavior is not required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 301c036a-80af-4e1a-b048-32b619709a21
📒 Files selected for processing (2)
flixopt/comparison.pytests/test_comparison.py
FBumann
force-pushed
the
comparison-mapping-ergonomics
branch
2 times, most recently
from
10095b4 to
633a59e
Compare
linopy 0.6.x breaks with xarray 2026.3+ (TypeError on Dataset constructor). Keep the release dependency range unchanged; only pin for dev/CI until linopy ships a fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Raise TypeError if flow_systems is not a list, or if any item is not a FlowSystem instance — replaces the cryptic AttributeError that would otherwise surface later when attributes like .name or .solution are accessed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pyproject.toml (1)
80-80: Consider removing thexarray<2026.3constraint — linopy v0.6.6 (latest) has no upper bound.The code change is sound as-is and scoped to dev installs. However, linopy's latest release (v0.6.6, March 2026) already specifies
xarray>=2024.2.0with no upper bound, suggesting the xarray 2026.3+ compatibility fix has shipped. The TODO can likely be resolved and the constraint removed in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 80, The dependency pin "xarray<2026.3" in pyproject.toml is now unnecessary because linopy v0.6.6 drops the upper bound; remove the "<2026.3" upper bound from the xarray specification (i.e., replace "xarray<2026.3" with simply "xarray" or "xarray>=<minimum-version>" as appropriate) and delete the accompanying TODO comment so the dev install no longer constrains xarray to <2026.3; update any related comment to reflect that linopy v0.6.6 provides compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flixopt/comparison.py`:
- Around line 163-171: The element-type validation for the Comparison input is
performed after the minimum-length check, causing a ValueError when a single
non-FlowSystem is passed; change the order so the non_fs check runs before the
len(flow_systems) < 2 check: move the creation/inspection of non_fs (the list
comprehension using isinstance(..., FlowSystem)) and the subsequent TypeError
raise to precede the length check on flow_systems, ensuring flow_systems and
FlowSystem are validated first and the correct TypeError is raised for invalid
elements.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 80: The dependency pin "xarray<2026.3" in pyproject.toml is now
unnecessary because linopy v0.6.6 drops the upper bound; remove the "<2026.3"
upper bound from the xarray specification (i.e., replace "xarray<2026.3" with
simply "xarray" or "xarray>=<minimum-version>" as appropriate) and delete the
accompanying TODO comment so the dev install no longer constrains xarray to
<2026.3; update any related comment to reflect that linopy v0.6.6 provides
compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 202ece00-20ff-49e9-9cf3-5ffc1fdc50e3
📒 Files selected for processing (3)
flixopt/comparison.pypyproject.tomltests/test_comparison.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_comparison.py
Check that each element is a FlowSystem before the minimum-length check, so `Comparison([not_a_flow_system])` surfaces the real problem (wrong type) instead of "requires at least 2 FlowSystems". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Comparison.__iter__now yields case names (keys) instead of(name, FlowSystem)pairs, matching thedict/Mappingconvention.keys(),values(), anditems()methods returning the usual dict views.Breaking change
Code that relied on
for name, fs in comp:should switch tofor name, fs in comp.items():. A grep of the repo found no internal call sites using the old pattern.Test plan
pytest tests/test_comparison.py::TestComparisonContainerProtocol— 16 passed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactoring
Bug Fixes
Tests
Chores