Feature/open mc sphere output#531
Merged
dodu94 merged 12 commits intoMay 21, 2026
Merged
Conversation
…hmarks in order to handle normalisation by atomic density.
…' would have been rescaled.
…eturned, alongside a test to ensure tally data attribute is the correct datatype for the OpenmcSphereSimOutput.
…SimOutput), to handle normalisation of T production, He ppm production, DPA production in Sphere tallies within openmc.
Contributor
WalkthroughAdds OpenMC Sphere benchmark support with atomic-density-based normalization for selected tallies, refactors tally preparation into a reusable helper, fixes a heating/damage-energy normalization detection condition, wires a Sphere-specific output class into RawProcessor, and adds config and tests for Sphere outputs. ChangesOpenMC Sphere Support
Sequence Diagram(s)sequenceDiagram
participant RawProcessor
participant OpenMCSphereSimOutput
participant SphereInput as Sphere Input
participant TallyPrep as _prep_tally
participant TallyDF as Tally DataFrames
RawProcessor->>OpenMCSphereSimOutput: instantiate(sim_folder)
OpenMCSphereSimOutput->>SphereInput: load materials.xml
SphereInput-->>OpenMCSphereSimOutput: atomic_density
OpenMCSphereSimOutput->>TallyPrep: _prep_tally(tally)
TallyPrep-->>TallyDF: prepared DataFrame
OpenMCSphereSimOutput->>TallyDF: normalize Value/Error by atomic_density (IDs 14,24,34)
OpenMCSphereSimOutput->>TallyDF: normalize Error by Value (if present)
TallyDF-->>RawProcessor: processed tallies dict
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/jade/post/sim_output.py`:
- Around line 374-375: _in _prep_tally, the code unconditionally calls
tally.sort_values(filters) which fails when filters is empty or none of the
filter columns exist; guard the sort by checking that filters is non-empty and
that at least one element of filters exists in tally.columns (e.g., compute
valid_filters = [f for f in filters if f in tally.columns]) and only call
tally.sort_values(valid_filters) when valid_filters is non-empty, otherwise skip
sorting but still call reset_index(drop=True) on the original tally; update
references around sorted_tally, tally, and filters accordingly.
- Around line 473-476: The code currently hard-codes materials[1] to fetch
atomic densities; instead locate the single material dynamically from the dict
returned by get_all_materials() (e.g., take the first value or iterate until one
is found) and call get_nuclide_atom_densities() on that material, then sum its
values to assign self.atomic_density; update the logic around materials,
get_all_materials(), get_nuclide_atom_densities(), and self.atomic_density to
use the first/only material rather than the literal key 1.
In `@tests/post/test_sim_output.py`:
- Around line 58-67: The TestOpenMCSphereSimOutput tests need to be skipped when
OpenMC isn't installed: add a class-level skip guard so the whole
TestOpenMCSphereSimOutput is skipped if the openmc package is unavailable (e.g.,
add import pytest and import importlib.util then decorate the class with
`@pytest.mark.skipif`(importlib.util.find_spec("openmc") is None, reason="OpenMC
not installed"), which will skip test_openmc_sphere_tallydata and
test_atomic_density (and avoid running fixtures like openmc_sphere_sim_output)
in CI environments without OpenMC.
🪄 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: b5c6cf94-268c-4502-b8c1-28ac57cf7d32
📒 Files selected for processing (7)
src/jade/helper/openmc.pysrc/jade/post/raw_processor.pysrc/jade/post/sim_output.pysrc/jade/resources/default_cfg/benchmarks_pp/raw/openmc/Sphere.yamltests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Sphere/Sphere_m101/geometry.xmltests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Sphere/Sphere_m101/materials.xmltests/post/test_sim_output.py
Comment on lines
+374
to
+375
| sorted_tally = tally.sort_values(filters) | ||
| sorted_tally = sorted_tally.reset_index(drop=True) |
Contributor
There was a problem hiding this comment.
Guard sorting when no filter columns are present.
_prep_tally sorts by filters unconditionally; if a tally has none of the supported axes, this path raises at runtime and drops processing for that tally.
Proposed fix
- sorted_tally = tally.sort_values(filters)
- sorted_tally = sorted_tally.reset_index(drop=True)
+ sorted_tally = tally
+ if filters:
+ sorted_tally = sorted_tally.sort_values(filters)
+ sorted_tally = sorted_tally.reset_index(drop=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sorted_tally = tally.sort_values(filters) | |
| sorted_tally = sorted_tally.reset_index(drop=True) | |
| sorted_tally = tally | |
| if filters: | |
| sorted_tally = sorted_tally.sort_values(filters) | |
| sorted_tally = sorted_tally.reset_index(drop=True) |
🤖 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/sim_output.py` around lines 374 - 375, _in _prep_tally, the
code unconditionally calls tally.sort_values(filters) which fails when filters
is empty or none of the filter columns exist; guard the sort by checking that
filters is non-empty and that at least one element of filters exists in
tally.columns (e.g., compute valid_filters = [f for f in filters if f in
tally.columns]) and only call tally.sort_values(valid_filters) when
valid_filters is non-empty, otherwise skip sorting but still call
reset_index(drop=True) on the original tally; update references around
sorted_tally, tally, and filters accordingly.
Codecov Report❌ Patch coverage is
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
dodu94
requested changes
May 20, 2026
Member
dodu94
left a comment
dodu94
left a comment
There was a problem hiding this comment.
Please add a test in excel post-processor between openmc and mcnp results
| statepoint_found = True | ||
| return statepoint_found | ||
|
|
||
| def _prep_tally(self, filter_lookup, tally: pd.DataFrame) -> None: |
Member
There was a problem hiding this comment.
correct the typing, filter_lookup is missing and I think the return type is a dataframe
| Returns | ||
| ------- | ||
| None. | ||
|
|
Member
There was a problem hiding this comment.
use super.init() to call the init function of the parent and add after that the extra lines needed for this class specifically
Member
|
Also: remember to add the new sphere input to the IAEA open benchmark repo |
…e declaration of atomic_density, which is required for create_dataframes.
Contributor
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/sim_output.py (1)
496-554: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCollapse
OpenMCSphereSimOutput._create_dataframesonto the parent implementation.This override duplicates ~95% of
OpenMCSimOutput._create_dataframes(filter_lookup, unusedcolumns, the_prep_tallyloop, and theError = Error / Valuestep). Mathematically the density step cancels in the relative-error conversion:
- RR path:
Value' = V/d,Error' = (E/d) / (V/d) = E/V- Parent path:
Error' = E/VSo the only net difference for RR tallies is
Value /= atomic_density. The whole override can be reduced to a post-processing pass over the parent's output, which also fixes the misleading comment ("Need to generate atomic density for a given Sphere input" — already done in__init__) and the deadelse: pass.Proposed refactor
def _create_dataframes( self, tallies: dict ) -> tuple[dict[int, pd.DataFrame], dict[int, pd.DataFrame]]: """ Function to create dataframes in JADE format from OpenMC dataframes. - - Parameters - ---------- - tallies : dict - Dictionary of OpenMC tally dataframes, indexed by tally number - - Returns - ------- - tallydata : dict[int, pd.DataFrame] - Dictionary of JADE formatted tally dataframes, indexed by tally number - totalbin : dict[int, None]] - Dictionary of JADE formatted total tally values, each are None for OpenMC + Sphere-specific override: normalises Value of the He/T/DPA reaction-rate + tallies (IDs 14, 24, 34) by total atomic density. Relative-error + conversion is unaffected and is handled by the parent. """ - tallydata = {} - totalbin = {} - filter_lookup = { - "cell": "Cells", - "surface": "Segments", - "energy high [eV]": "Energy", - "time": "Time", - "mean": "Value", - "std. dev.": "Error", - } - columns = [ ... ] - for id, tally in tallies.items(): - sorted_tally = self._prep_tally(filter_lookup, tally) - - # If tally is Sphere, and is tally 14,24,34 then need to normalise by atomic density - # Need to generate atomic density for a given Sphere input - RR_tally_IDs = [14, 24, 34] - if id in RR_tally_IDs: - sorted_tally["Value"] = sorted_tally["Value"] / self.atomic_density - sorted_tally["Error"] = sorted_tally["Error"] / self.atomic_density - else: - pass - - if "Value" in sorted_tally.columns and "Error" in sorted_tally.columns: - sorted_tally["Error"] = sorted_tally["Error"] / sorted_tally["Value"] - - tallydata[id] = sorted_tally - totalbin[id] = None - return tallydata, totalbin + tallydata, totalbin = super()._create_dataframes(tallies) + # He/T/DPA reaction-rate tallies need normalisation by atomic density + # to be comparable to MCNP output. + rr_tally_ids = (14, 24, 34) + for tally_id in rr_tally_ids: + df = tallydata.get(tally_id) + if df is not None and "Value" in df.columns: + df["Value"] = df["Value"] / self.atomic_density + return tallydata, totalbin🤖 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/sim_output.py` around lines 496 - 554, This override of OpenMCSphereSimOutput._create_dataframes duplicates almost the entire parent OpenMCSimOutput._create_dataframes; instead call the parent implementation to get tallydata and totalbin, then apply the single Sphere-specific mutation: for RR_tally_IDs = [14, 24, 34] divide tallydata[id]["Value"] by self.atomic_density (leave errors as produced by the parent), remove the dead else: pass and duplicated helpers (filter_lookup, columns, _prep_tally usage), and retain references to self.atomic_density and RR_tally_IDs so the post-processing is obvious.
🧹 Nitpick comments (1)
src/jade/post/sim_output.py (1)
425-437: ⚡ Quick winUnused
columnslist is dead code.The local
columnslist defined here is never referenced in the function. Same dead block is duplicated at lines 524-536 in the Sphere override. Either drop it, or wire it back in if it was meant to enforce a column order on the output (in which case it should also include"Cosine"-style ordering after_remove_constant_columns).Proposed fix
tallydata = {} totalbin = {} filter_lookup = { ... } - columns = [ - "Cells", - "User", - "Segments", - "Cosine", - "Energy", - "Time", - "Cor C", - "Cor B", - "Cor A", - "Value", - "Error", - ] for id, tally in tallies.items():🤖 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/sim_output.py` around lines 425 - 437, The local variable "columns" in sim_output.py is dead/duplicated and should be removed or re-used: either delete both occurrences (the one at the top-level function and the duplicate in the Sphere override) to eliminate dead code, or if you intended to enforce output column ordering, apply this list after calling _remove_constant_columns (use it to reorder the resulting columns and include the "Cosine"-style ordering) and remove the duplicate list in the Sphere override; update the function(s) that build the output table to use that ordering rather than leaving the list unused.
🤖 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/sim_output.py`:
- Around line 496-554: This override of OpenMCSphereSimOutput._create_dataframes
duplicates almost the entire parent OpenMCSimOutput._create_dataframes; instead
call the parent implementation to get tallydata and totalbin, then apply the
single Sphere-specific mutation: for RR_tally_IDs = [14, 24, 34] divide
tallydata[id]["Value"] by self.atomic_density (leave errors as produced by the
parent), remove the dead else: pass and duplicated helpers (filter_lookup,
columns, _prep_tally usage), and retain references to self.atomic_density and
RR_tally_IDs so the post-processing is obvious.
---
Nitpick comments:
In `@src/jade/post/sim_output.py`:
- Around line 425-437: The local variable "columns" in sim_output.py is
dead/duplicated and should be removed or re-used: either delete both occurrences
(the one at the top-level function and the duplicate in the Sphere override) to
eliminate dead code, or if you intended to enforce output column ordering, apply
this list after calling _remove_constant_columns (use it to reorder the
resulting columns and include the "Cosine"-style ordering) and remove the
duplicate list in the Sphere override; update the function(s) that build the
output table to use that ordering rather than leaving the list unused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d275765-b8d8-4dd2-921a-769d1d91e4a6
📒 Files selected for processing (2)
src/jade/post/sim_output.pytests/post/test_sim_output.py
dodu94
approved these changes
May 21, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/jade/post/sim_output.py (1)
390-391:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard sorting when no filter columns are present.
tally.sort_values(filters)is still invoked unconditionally; for any tally whose dataframe has none ofcell/surface/energy high [eV]/time,filtersis empty and pandas will raise on the sort, dropping that tally from processing.Proposed fix
- sorted_tally = tally.sort_values(filters) - sorted_tally = sorted_tally.reset_index(drop=True) + sorted_tally = tally + if filters: + sorted_tally = sorted_tally.sort_values(filters) + sorted_tally = sorted_tally.reset_index(drop=True)🤖 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/sim_output.py` around lines 390 - 391, The code unconditionally calls tally.sort_values(filters) which fails when filters is empty or none of the filter names exist; update the block around tally, filters and sorted_tally so you only call tally.sort_values(...) when there is at least one valid column to sort by: compute the intersection between filters and tally.columns (or check filters truthiness), and if the resulting list is non-empty call tally.sort_values(valid_filters) then reset_index, otherwise skip sort and just do tally.reset_index(drop=True) (references: variables tally, filters and the call to sort_values producing sorted_tally).
🧹 Nitpick comments (1)
src/jade/post/sim_output.py (1)
496-554: ⚡ Quick winReduce duplication by delegating to the parent
_create_dataframes.The override copy-pastes the entire base implementation just to apply an extra
Value/Errordivision byself.atomic_densityfor tally IDs 14/24/34. Note that dividingErrorbyatomic_density(line 545) has no net effect: line 550 immediately rewritesErrorasError / Value, and theatomic_densityfactor cancels between numerator and denominator. So the only quantity that actually needs adjustment isValue, which can be done after delegating to the base method.This avoids re-declaring
filter_lookup/columns/the loop, and keeps the two classes in sync if the base post-processing evolves.Proposed refactor
def _create_dataframes( self, tallies: dict ) -> tuple[dict[int, pd.DataFrame], dict[int, pd.DataFrame]]: """ Function to create dataframes in JADE format from OpenMC dataframes. ... """ - tallydata = {} - totalbin = {} - filter_lookup = { - "cell": "Cells", - "surface": "Segments", - "energy high [eV]": "Energy", - "time": "Time", - "mean": "Value", - "std. dev.": "Error", - } - columns = [ - "Cells", - "User", - "Segments", - "Cosine", - "Energy", - "Time", - "Cor C", - "Cor B", - "Cor A", - "Value", - "Error", - ] - for id, tally in tallies.items(): - sorted_tally = self._prep_tally(filter_lookup, tally) - - # If tally is Sphere, and is tally 14,24,34 then need to normalise by atomic density - # Need to generate atomic density for a given Sphere input - RR_tally_IDs = [14, 24, 34] - if id in RR_tally_IDs: - sorted_tally["Value"] = sorted_tally["Value"] / self.atomic_density - sorted_tally["Error"] = sorted_tally["Error"] / self.atomic_density - else: - pass - - if "Value" in sorted_tally.columns and "Error" in sorted_tally.columns: - sorted_tally["Error"] = sorted_tally["Error"] / sorted_tally["Value"] - - tallydata[id] = sorted_tally - totalbin[id] = None - return tallydata, totalbin + # Normalize the raw `mean` (Value) by atomic density for the DPA/He/T + # production tallies before the base class converts Error to relative. + # Dividing Error here as well would be cancelled out by the subsequent + # Error/Value relative-error calculation, so only Value needs adjusting. + RR_tally_IDs = {14, 24, 34} + adjusted = {} + for tid, tally in tallies.items(): + if tid in RR_tally_IDs and "mean" in tally.columns: + tally = tally.copy() + tally["mean"] = tally["mean"] / self.atomic_density + adjusted[tid] = tally + return super()._create_dataframes(adjusted)🤖 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/sim_output.py` around lines 496 - 554, The overridden _create_dataframes currently duplicates the base implementation; instead call the parent implementation (e.g., parent_tallydata, parent_totalbin = super()._create_dataframes(tallies)) to get tallydata and totalbin, then apply the RR_tally_IDs = [14,24,34] adjustment by dividing only tallydata[id]["Value"] by self.atomic_density for those ids (do not re-divide Error since the base method already converts Error to a relative error), remove the duplicated filter_lookup/columns/loop, and keep returning the modified tallydata and totalbin.
🤖 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.
Duplicate comments:
In `@src/jade/post/sim_output.py`:
- Around line 390-391: The code unconditionally calls tally.sort_values(filters)
which fails when filters is empty or none of the filter names exist; update the
block around tally, filters and sorted_tally so you only call
tally.sort_values(...) when there is at least one valid column to sort by:
compute the intersection between filters and tally.columns (or check filters
truthiness), and if the resulting list is non-empty call
tally.sort_values(valid_filters) then reset_index, otherwise skip sort and just
do tally.reset_index(drop=True) (references: variables tally, filters and the
call to sort_values producing sorted_tally).
---
Nitpick comments:
In `@src/jade/post/sim_output.py`:
- Around line 496-554: The overridden _create_dataframes currently duplicates
the base implementation; instead call the parent implementation (e.g.,
parent_tallydata, parent_totalbin = super()._create_dataframes(tallies)) to get
tallydata and totalbin, then apply the RR_tally_IDs = [14,24,34] adjustment by
dividing only tallydata[id]["Value"] by self.atomic_density for those ids (do
not re-divide Error since the base method already converts Error to a relative
error), remove the duplicated filter_lookup/columns/loop, and keep returning the
modified tallydata and totalbin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b680ab4-4b91-4127-be86-ee26d819a600
📒 Files selected for processing (1)
src/jade/post/sim_output.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request includes various changes with the combined effort to implement additional tallies into the OpenMC Sphere benchmarks.
To make the new tallies comparible to the MCNP results they must be normalised by the atomic density of the material. It would be impractical to implement this in the raw.yaml file for the OpenMC sphere benchmark for ever instance of the Sphere benchmark.
Instead, the atomic densities are being retrieved from the geometry.xml file and being folded into the tally results.
To facilitate this the following changes have been made:
- self.atomic_density is retrieved in innit function
- Differing logic in create_dataframe function that divides by atomic density
Summary by CodeRabbit
New Features
Bug Fixes
Tests