534 bug jade cannot handle total heating tallies no particle filter

[sbradnam]

• This pull request fixes the logic associated with processing OpenMC heating tallies. Previuously, total heating tallies, where a heating score is recorded by no openmc.ParticleFilter is applied, would not be processed by JADE. In addition, the logic has been revised, to ensure all cases where photon, electron on positron filters on heating tallies with the same openmc.CellFilter are combined together.
• The test suite was also failing on UKAEA HPC, in test_app.py, but still passing CI. This has been updated to fix the test to pass on UKAEA HPC.
• Finally, documentation in the developers guide has bene updated to explain the nuances described above in OpenMC heating tally definitions.

Summary by CodeRabbit

• Documentation

  • Added guidance on configuring total and photon heating tallies in OpenMC benchmarks.

• Bug Fixes

  • Improved heating tally aggregation to properly handle cases with missing particle filters.
  • Enhanced combination logic for photon heating contributions from electron and positron tallies.

[coderabbitai]

Warning

Review limit reached

@sbradnam, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 33 minutes and 43 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ed836243-2955-446a-aa7a-e4c70d31411b

📥 Commits

Reviewing files that changed from the base of the PR and between bf18872 and 94ea684.

📒 Files selected for processing (2)

• docs/source/dev/add_benchmark/where_inputs.rst
• src/jade/helper/openmc.py

Walkthrough

This PR refactors the heating tally combination logic in OpenMCStatePoint to safely handle missing ParticleFilters and correctly aggregate photon, electron, and positron tallies using quadrature for standard deviation. Documentation and test assertions are updated to reflect the new behavior, and a test ordering issue is stabilized.

Changes

Heating Tally Improvements

Layer / File(s)	Summary
Heating tally combination refactor and documentation src/jade/helper/openmc.py, docs/source/dev/add_benchmark/where_inputs.rst	_combine_heating_tallies now safely locates ParticleFilter (treating missing as None), routes tallies into neutron/photon collections based on particle bins, and combines photon/electron/positron contributions by matching CellFilter objects with quadrature std dev aggregation. Documentation note explains the pattern for users modeling photon heating.
Tally combination test assertions tests/helper/test_openmc.py	Test assertions validate the "total" nuclide label and update expected pytest.approx values for mean and std-dev derived ratios.
Test ordering stabilization tests/app/test_app.py	test_continue_run_sphere now sorts command tuples before asserting SBATCH script contents for order-independent validation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

• JADE-V-V/JADE#534: Fixes the bug where missing ParticleFilter on total heating tallies caused errors and correctly combines photon/electron/positron tallies as documented.

Possibly related PRs

• JADE-V-V/JADE#442: Both PRs document and implement OpenMC heating-tally patterns for summing photon/electron/positron contributions into MCNP-equivalent tallies.

Suggested labels

bug

Suggested reviewers

• dodu94
• alexvalentine94

Poem

A rabbit hops through tallies bright,
Where photons dance with electrons light,
No filters? Safe! Quadrature flows,
As heating sums through cells it goes,
Tests sorted, assertions aligned—
A tidy fix for all combined! 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main issue: fixing JADE's handling of total heating tallies without particle filters, which is the core problem solved by the refactored tally combination logic.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 534-bug---jade-cannot-handle-total-heating-tallies-no-particle-filter

---

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/helper/openmc.py	88.97% <100.00%> (+0.46%)	⬆️

🚀 New features to boost your workflow:

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

tests/app/test_app.py (1)

225-229: ⚡ Quick win

Sort by run folder identity, not rendered script text.

x[1] is the generated batch script body, so ordering can change if template text changes. For stable order-independent validation, sort by the run path tuple element (e.g., x[2]) or build assertions keyed by folder name instead of fixed positions.

Suggested adjustment

-        command_sorted = sorted(command, key=lambda x: x[1])
+        command_sorted = sorted(command, key=lambda x: str(x[2]))

         assert "#!/bin/sh\n\n#SBATCH" in command_sorted[0][1]
         assert "Sphere_dummy1" in command_sorted[0][1]
         assert "Sphere_m101" in command_sorted[1][1]

🤖 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/app/test_app.py` around lines 225 - 229, The test sorts generated
command tuples by the script body (command_sorted = sorted(command, key=lambda
x: x[1])) which is brittle; change the sort key to the run folder identity
element (e.g., use key=lambda x: x[2] or another tuple index that holds the run
path) or instead build assertions by looking up tuples by their folder name
rather than relying on fixed positions; update the assertions that reference
command_sorted[0][1] / command_sorted[1][1] to reference the correct tuple by
folder identity (using x[2] or a dict lookup) so ordering is stable when
template text changes.

🤖 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 `@docs/source/dev/add_benchmark/where_inputs.rst`:
- Around line 80-82: Fix two typos in the OpenMC guidance sentence that
describes adding photon heating tallies: change "seperate" to "separate" and
"repsonse" to "response" in the line containing the phrase "add 3 seperate
tallies" / "produce an equivalent repsonse" (search for the sentence mentioning
"openmc.CellFilter" and "openmc.ParticleFilter" to locate the exact spot).

In `@src/jade/helper/openmc.py`:
- Around line 583-589: The predicate `if "electron" or "positron" in
particle_filter.bins:` is incorrect and always truthy, causing
non-electron/positron tallies to be merged into heating_tallies_df; change the
condition in the loop that uses particle_filter.bins (the block that calls
self._get_tally_data and updates heating_tallies_df[id]["mean"] and ["std.
dev."]) to explicitly test membership for both tokens (e.g. `"electron" in
particle_filter.bins or "positron" in particle_filter.bins"` or use any(x in
particle_filter.bins for x in ("electron","positron"))), so only
electron/positron bins contribute to the combined mean/std-dev calculations.

---

Nitpick comments:
In `@tests/app/test_app.py`:
- Around line 225-229: The test sorts generated command tuples by the script
body (command_sorted = sorted(command, key=lambda x: x[1])) which is brittle;
change the sort key to the run folder identity element (e.g., use key=lambda x:
x[2] or another tuple index that holds the run path) or instead build assertions
by looking up tuples by their folder name rather than relying on fixed
positions; update the assertions that reference command_sorted[0][1] /
command_sorted[1][1] to reference the correct tuple by folder identity (using
x[2] or a dict lookup) so ordering is stable when template text changes.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1e115135-b0e2-41df-9ea3-7aa33edb39c4

📥 Commits

Reviewing files that changed from the base of the PR and between daff5ec and bf18872.

📒 Files selected for processing (5)

• docs/source/dev/add_benchmark/where_inputs.rst
• src/jade/helper/openmc.py
• tests/app/test_app.py
• tests/helper/test_openmc.py
• tests/post/resources/openmc/statepoint.10.h5

[dodu94]

Thanks Steve, have a look at the comments from CodeRabbit, they may be worth to implement. Does this PR requires to modify existing OpenMC inputs of JADE?

[sbradnam]

Thanks Steve, have a look at the comments from CodeRabbit, they may be worth to implement. Does this PR requires to modify existing OpenMC inputs of JADE?

No it just catches a case which would have been missed previously. None of the existing OpenMC benchmarks in JADE have total heating tallies. However C-Model does so this has been implimebted to support the addition of OpenMC C-Model later on.