Avoid scalar divide warnings raised when generating raw .csv files#533
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a division-by-zero guard to the ChangesError Guard and Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jade/post/manipulate_tally.py (1)
168-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick winZero-denominator handling is incomplete in
groupby(by=="all"path still divides unguarded).You added a guard for grouped subsets, but Line 170 can still divide by zero when
tally["Value"].sum() == 0, so the warning can still occur forby="all".Proposed fix
if by == "all": grouped = tally # Error propagation considering that tally["Error"] are relative errors # Valid both for sum and mean + total_value = tally["Value"].sum() error = pd.Series( - np.sqrt(((tally["Error"] * tally["Value"]) ** 2).sum()) - / tally["Value"].sum(), + ( + np.sqrt(((tally["Error"] * tally["Value"]) ** 2).sum()) / total_value + if total_value != 0 + else 0 + ), name="Error", )Also applies to: 184-185
🤖 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 `@src/jade/post/manipulate_tally.py` around lines 168 - 172, The calculation of error in the "by=='all'" path does an unguarded divide by tally["Value"].sum() (variable error created from tally) which can be zero; change this to compute a denom = tally["Value"].sum() and if denom == 0 return a safe pd.Series (e.g. pd.Series(0.0, name="Error") or pd.Series(np.nan, name="Error")) else perform the original sqrt(...) / denom calculation; apply the same zero-denominator guard to the analogous computation referenced at lines 184-185 so both the top-level error assignment and the grouped-case error use the same denom check.
🧹 Nitpick comments (1)
tests/post/test_manipulate_tally.py (1)
225-234: ⚡ Quick winAdd a zero-total
by="all"regression case to actually lock in the warning fix.
len(recwarn) == 0is good, but this test never hitsby="all"withValue.sum()==0, which is the remaining warning-prone path. Add a small all-zero dataset case forgroupby(..., "all", "sum")and assert no warnings plusError == 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/post/test_manipulate_tally.py` around lines 225 - 234, Add a small all-zero dataset case to exercise the by="all" sum path: create a DataFrame (e.g., df_zero) where the Value column sums to 0 and corresponding Error inputs are zeros, call groupby(df_zero.copy(), "all", "sum"), assert that the resulting "Value" is 0, "Error" is 0, and len(recwarn) == 0 to ensure no warning is emitted; place this directly after the existing groupby(..., "all", "sum") assertions and reference the same groupby function under test.
🤖 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.
Outside diff comments:
In `@src/jade/post/manipulate_tally.py`:
- Around line 168-172: The calculation of error in the "by=='all'" path does an
unguarded divide by tally["Value"].sum() (variable error created from tally)
which can be zero; change this to compute a denom = tally["Value"].sum() and if
denom == 0 return a safe pd.Series (e.g. pd.Series(0.0, name="Error") or
pd.Series(np.nan, name="Error")) else perform the original sqrt(...) / denom
calculation; apply the same zero-denominator guard to the analogous computation
referenced at lines 184-185 so both the top-level error assignment and the
grouped-case error use the same denom check.
---
Nitpick comments:
In `@tests/post/test_manipulate_tally.py`:
- Around line 225-234: Add a small all-zero dataset case to exercise the
by="all" sum path: create a DataFrame (e.g., df_zero) where the Value column
sums to 0 and corresponding Error inputs are zeros, call groupby(df_zero.copy(),
"all", "sum"), assert that the resulting "Value" is 0, "Error" is 0, and
len(recwarn) == 0 to ensure no warning is emitted; place this directly after the
existing groupby(..., "all", "sum") assertions and reference the same groupby
function under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 866e6eda-139b-4d2a-863c-0677ea6d5e79
📒 Files selected for processing (2)
src/jade/post/manipulate_tally.pytests/post/test_manipulate_tally.py
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
2 participants
This pull requests implements a solution for the warning repeatedly raised when generating raw results:
The warning was traced back to the group_by function in the manipulate_tally.py file. Accordingly, the following modifications were implemented:
No additional dependencies were introduced.
Summary by CodeRabbit
Bug Fixes
Tests