fix(stats): threshold not applied along breakdown dimension in plot.effects (#719)#720
fix(stats): threshold not applied along breakdown dimension in plot.effects (#719)#720FBumann wants to merge 3 commits into
Conversation
`stats.plot.effects(by='component'|'contributor')` lays entities out along a coordinate of a single variable (e.g. `costs`) rather than as separate data variables. The threshold filter only dropped whole data variables, so non-invested components with a ~0 contribution were still shown (#719). Generalize the filter to also drop small entries along the breakdown dimension, split it into named helpers, and rename it `_drop_small` since it no longer filters variables only. Adds a parametrized regression test. Fixes #719 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe small-value threshold filtering logic in the statistics accessor was refactored into ChangesThreshold filtering refactor
Estimated code review effort: 2 (Simple) | ~12 minutes Related issues: Suggested reviewers: FBumann PoemA rabbit hopped through datasets wide, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@flixopt/statistics_accessor.py`:
- Around line 283-302: Both `_drop_small_data_vars` and `_drop_small_along_dim`
currently fall back to returning the original dataset when nothing meets the
threshold, which defeats filtering entirely. Change these helpers so the empty
case returns an empty filtered result instead of `ds` (for example, an empty
selection over `data_vars` or `dim`), while keeping the existing behavior when
some items survive. Add or extend a regression test around the
`effects`/`by='component'` or `by='contributor'` path to cover the
all-below-threshold case and verify nothing is reintroduced.
🪄 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: 38aff483-09c1-4083-80c8-898bd1937cd6
📒 Files selected for processing (2)
flixopt/statistics_accessor.pytests/plotting/test_solution_and_plotting.py
When every entry along the breakdown dimension is below threshold, _drop_small_along_dim returned the unfiltered dataset, silently ignoring the user's threshold (an all-below-threshold effects breakdown showed everything). Return the empty selection instead. The full effects render handles the empty dataset without crashing (verified across all aspects / by / effect combos). Scope: only the new dim-path helper is changed. The pre-existing _drop_small_data_vars fallback shared by 8 other plot methods is left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
flow_system.stats.plot.effects(by='component', threshold=...)still shows non-invested components. In the reproducer from #719, sourceS2never gets invested (zero cost contribution) yet remains in the plot despitethreshold=1.0.Root cause: with
by='component'/by='contributor', the breakdown lays entities out along a coordinate dimension of a single data variable (e.g.costs), not as separate data variables. The threshold filter (_filter_small_variables) only dropped whole data variables —costsas a whole is well above threshold, so nothing was removed and the zero-costS2survived.Fix
Generalize the filter to optionally also drop small entries along a coordinate dimension:
_drop_small_data_vars(whole variables) and_drop_small_along_dim(entries along a coordinate) — so the logic is self-documenting instead of commented.dim: str | list[str] | None;plot.effectspasses the active breakdown dimension._filter_small_variables→_drop_small, since it no longer filters variables only.Only
plot.effectscollapses entities into a coordinate dimension; every other threshold call site keeps one entity per data variable, so per-variable filtering there is already correct and is unchanged.Tests
Added a parametrized regression test (
by='component'andby='contributor') building the issue's scenario. It fails onmainand passes with this change.threshold=Nonestill keeps the zero-cost entry. Full plotting + comparison suites pass (111 tests).Fixes #719
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests