Fixing error handling bug outlined in issue 479

[jude-moo]

Bug fix for #479.

Removed the lines that was dividing the relative errors by mass/volume.

Summary by CodeRabbit

• Bug Fixes

  • Corrected per-cell normalization so reported error values are no longer altered during value normalization, yielding consistent error reporting.

• Tests

  • Updated unit test expectations to reflect the corrected error values for volume and mass normalization.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Removed per-cell error updates from the volume() and mass() normalization routines; only tally["Value"] is divided by per-cell volumes/masses now, and tally["Error"] is no longer modified during those loops. Tests updated to reflect the unchanged error values.

Changes

Cohort / File(s)	Summary
Tally normalization src/jade/post/manipulate_tally.py	Removed in-loop updates to tally["Error"] in volume() and mass(); only tally["Value"] is normalized per cell.
Tests updated tests/post/test_manipulate_tally.py	Adjusted expected Error assertion (first element) from 0.05 to 0.1 to match new behavior; Value assertions unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Removing tally factors, integrating volume and mass divisors into raw config yaml #459 — Modifies the same volume()/mass() normalization logic in src/jade/post/manipulate_tally.py; touches how Value and Error are divided per cell.

Suggested reviewers

• alexvalentine94
• dodu94

Poem

🐰 I hopped through loops at break of day,
I left the errors graceful in play,
Values scaled to each small cell,
Quiet changes—numbers dwell,
🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically references the fix for issue 479 related to error handling, which aligns with the main changes removing error propagation in per-cell normalization.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f96e865 and ef3321f.

📒 Files selected for processing (1)

• tests/post/test_manipulate_tally.py

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: mcampos16
Repo: JADE-V-V/JADE PR: 421
File: src/jade/post/manipulate_tally.py:265-271
Timestamp: 2025-09-22T09:52:06.987Z
Learning: In the JADE codebase, for the cumulative_sum function in src/jade/post/manipulate_tally.py, do not add divide-by-zero guards during normalization. If the last value of a cumulative sum is 0, the resulting divide-by-zero error serves as a useful diagnostic that the tally recorded no results in any bins, which users should be made aware of rather than having it masked by silent error handling.

Learnt from: mcampos16
Repo: JADE-V-V/JADE PR: 421
File: src/jade/post/manipulate_tally.py:152-158
Timestamp: 2025-09-22T09:51:20.734Z
Learning: In the JADE codebase, when dealing with tally data in src/jade/post/manipulate_tally.py, avoid silently handling divide-by-zero cases in error propagation calculations. If tally["Value"].sum() == 0, the resulting warning/error serves as a useful diagnostic that the tally recorded no results, which users should be made aware of rather than having it masked.

📚 Learning: 2025-09-22T09:51:20.734Z

Learnt from: mcampos16
Repo: JADE-V-V/JADE PR: 421
File: src/jade/post/manipulate_tally.py:152-158
Timestamp: 2025-09-22T09:51:20.734Z
Learning: In the JADE codebase, when dealing with tally data in src/jade/post/manipulate_tally.py, avoid silently handling divide-by-zero cases in error propagation calculations. If tally["Value"].sum() == 0, the resulting warning/error serves as a useful diagnostic that the tally recorded no results, which users should be made aware of rather than having it masked.

Applied to files:

• tests/post/test_manipulate_tally.py

🔇 Additional comments (2)

tests/post/test_manipulate_tally.py (2)

383-393: LGTM!

The updated assertion correctly reflects the bug fix. When normalizing tally values by volume, the relative error should remain unchanged (0.1) rather than being incorrectly divided by the volume (which would yield 0.05). This aligns with proper error propagation for relative errors in Monte Carlo tallies.

---

396-406: LGTM!

Same correct fix as test_volume — the relative error remains at its original value (0.1) after mass normalization, which is the expected behavior when dividing values by a scalar normalization factor.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
src/jade/post/manipulate_tally.py	96.00% <ø> (-0.04%)	⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dodu94]

Indeed, volumes are constants so no uncertainty propagation/modification there. Thanks, Davide