feat(stats): add StatsAccessor.summary() headline KPI table#188
feat(stats): add StatsAccessor.summary() headline KPI table#188Shaan-alpha wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 57 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a new ChangesStats summary KPI table
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 (1)
test_stats_quick.py (1)
1-13: ⚡ Quick winMake this a real test that asserts the acceptance criterion.
This file is named
test_*but contains only module-level code withprint()and no assertions, so pytest collects/executes the build+solve at import time without verifying anything. The linked issue's acceptance criterion requires confirming the summary covers objective, effect totals, and full-load hours — assert those explicitly inside a test function.💚 Proposed test skeleton
import numpy as np import xarray as xr from fluxopt import FlowSystem, Flow, Effect def test_stats_summary_quickstart(): time = xr.DataArray(np.arange(2), dims=['time'], coords={'time': np.arange(2)}) sys = FlowSystem(time=time, dt=1.0) sys.build(flows=[Flow(id='f1', size=10)], effects=[Effect(id='cost')]) res = sys.solve() summary = res.stats.summary() assert 'objective' in summary assert 'effect_totals' in summary assert 'full_load_hours' in summary assert 'f1' in summary['full_load_hours'].coords['flow'].values🤖 Prompt for 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. In `@test_stats_quick.py` around lines 1 - 13, Convert the module-level script into a pytest test function (e.g., test_stats_summary_quickstart) that builds FlowSystem, calls solve(), and asserts the acceptance criteria rather than printing; specifically use FlowSystem, Flow(id='f1'), Effect(id='cost'), call res = sys.solve() and summary = res.stats.summary(), then assert that 'objective', 'effect_totals', and 'full_load_hours' are present in summary and that the flow id 'f1' appears in summary['full_load_hours'].coords['flow'].values.
🤖 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.
Nitpick comments:
In `@test_stats_quick.py`:
- Around line 1-13: Convert the module-level script into a pytest test function
(e.g., test_stats_summary_quickstart) that builds FlowSystem, calls solve(), and
asserts the acceptance criteria rather than printing; specifically use
FlowSystem, Flow(id='f1'), Effect(id='cost'), call res = sys.solve() and summary
= res.stats.summary(), then assert that 'objective', 'effect_totals', and
'full_load_hours' are present in summary and that the flow id 'f1' appears in
summary['full_load_hours'].coords['flow'].values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ff618c7-4f40-4321-868f-5cf2d50170df
📒 Files selected for processing (2)
src/fluxopt/stats.pytest_stats_quick.py
Shaan-alpha
force-pushed
the
feat/stats-summary
branch
from
470ea6a to
9723917
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_stats.py (1)
14-17: ⚡ Quick winStrengthen assertions to validate KPI content, not just variable presence.
These checks can pass even if variables are empty/misaligned. Please assert minimal semantics too (e.g.,
objectiveis scalar/non-null,effect_totalscontainscost, andfull_load_hoursforf1is finite/non-negative) so this test actually guards the acceptance criteria.Suggested assertion upgrade
assert 'objective' in summary assert 'effect_totals' in summary assert 'full_load_hours' in summary assert 'f1' in summary['full_load_hours'].coords['flow'].values + assert np.isfinite(summary['objective'].item()) + assert 'cost' in summary['effect_totals'].coords['effect'].values + assert np.isfinite(summary['full_load_hours'].sel(flow='f1').item()) + assert summary['full_load_hours'].sel(flow='f1').item() >= 0🤖 Prompt for 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. In `@tests/test_stats.py` around lines 14 - 17, The current assertions only check key presence; strengthen them by validating content semantics: assert that summary['objective'] is non-null and scalar/finite, that summary['effect_totals'] contains a 'cost' entry which is finite and non-negative, and that summary['full_load_hours'] for flow 'f1' (accessed via summary['full_load_hours'].coords['flow'].values and the corresponding data array) contains at least one value that is finite and >= 0; update the test in tests/test_stats.py to add these non-emptiness/finite/non-negative checks around the existing keys ('objective', 'effect_totals', 'full_load_hours') and the 'f1' flow to ensure the KPIs are meaningful.
🤖 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.
Nitpick comments:
In `@tests/test_stats.py`:
- Around line 14-17: The current assertions only check key presence; strengthen
them by validating content semantics: assert that summary['objective'] is
non-null and scalar/finite, that summary['effect_totals'] contains a 'cost'
entry which is finite and non-negative, and that summary['full_load_hours'] for
flow 'f1' (accessed via summary['full_load_hours'].coords['flow'].values and the
corresponding data array) contains at least one value that is finite and >= 0;
update the test in tests/test_stats.py to add these
non-emptiness/finite/non-negative checks around the existing keys ('objective',
'effect_totals', 'full_load_hours') and the 'f1' flow to ensure the KPIs are
meaningful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60277c2b-86f3-4916-82c0-a26777057e31
📒 Files selected for processing (2)
src/fluxopt/stats.pytests/test_stats.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fluxopt/stats.py
|
@coderabbitai resolve Feedback addressed! I've converted the module-level script into a proper pytest test function with assertions as suggested. |
|
(｡◕‿◕｡)🐇 ✅ Actions performedComments resolved. Approval is disabled; enable |
summary() raised `TypeError: len() of unsized object` whenever the result had no invested sizes, because Result.sizes is an empty 0-d DataArray in that (very common) case. Guard on the `flow` dim instead of len(), so fixed-size systems work too. Replace the placeholder test with a real end-to-end one: the previous test called a non-existent FlowSystem(time=..., dt=...) / build(flows=, effects=) API and was never executed (the CI has no pytest job), so it hid the bug above. The new test builds a minimal system via optimize() and asserts the KPIs carry content, not just keys. Renamed to test_stats_summary.py to avoid a basename clash with tests/math/test_stats.py that broke full-suite collection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed 7b7b63d, which goes a bit further than the assertion nitpick. While strengthening the test I found Changes in this commit:
Verified locally: full suite is 470 passed, 227 skipped, |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_stats_summary.py (1)
29-29: ⚡ Quick win
effect_totalsassertion only checks the key, not the value.The comment above says the KPIs should "carry meaningful content, not just keys," but unlike
objective(Line 28) andfull_load_hours(Line 34), this assertion only verifies thecostcoordinate label exists. A regression returningNaN/missing for thecosttotal would still pass. Add a finiteness check on the selected value to match intent.💚 Proposed assertion to verify content
assert 'cost' in summary['effect_totals'].coords['effect'].values + assert np.isfinite(summary['effect_totals'].sel(effect='cost').item())🤖 Prompt for 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. In `@tests/test_stats_summary.py` at line 29, The test currently only asserts the 'cost' coordinate exists in summary['effect_totals'] but not that its value is meaningful; update the test to also assert the selected cost total is finite/non-NaN (e.g., use numpy.isfinite or pandas.notna) by selecting the value via summary['effect_totals'].sel(effect='cost') (or equivalent indexing used elsewhere in the file) and asserting that the returned scalar is finite/not-NaN to match the checks applied to 'objective' and 'full_load_hours'.
🤖 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.
Nitpick comments:
In `@tests/test_stats_summary.py`:
- Line 29: The test currently only asserts the 'cost' coordinate exists in
summary['effect_totals'] but not that its value is meaningful; update the test
to also assert the selected cost total is finite/non-NaN (e.g., use
numpy.isfinite or pandas.notna) by selecting the value via
summary['effect_totals'].sel(effect='cost') (or equivalent indexing used
elsewhere in the file) and asserting that the returned scalar is finite/not-NaN
to match the checks applied to 'objective' and 'full_load_hours'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f081032-a7ac-4ffb-bf0a-e61749895127
📒 Files selected for processing (2)
src/fluxopt/stats.pytests/test_stats_summary.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fluxopt/stats.py
Match the finiteness checks already applied to objective and full_load_hours, so a NaN/missing cost total fails instead of passing on the key alone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Added in 169e748 — |
1 participant
Fixes #186. Adds summary() to StatsAccessor which returns an xr.Dataset containing the objective value, effect_totals, and per-flow full_load_hours.
Summary by CodeRabbit