Removing tally factors, integrating volume and mass divisors into raw config yaml #459
Conversation
WalkthroughAdds volume and mass tally modifiers across config, processing, and docs. Refactors OpenMC helpers into a unified OpenMCCellData with combined JSON/XML loading and mass derivation. Updates OpenMC statepoint and sim output APIs accordingly. Extends raw processing to inject volumes/masses. Updates benchmark configs and tests; adds OpenMC XML/JSON fixtures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant RP as RawProcessor
participant MOD as MOD_FUNCTIONS
participant SO as SimOutput (OpenMC)
participant SP as OpenMCStatePoint
participant CD as OpenMCCellData
RP->>SO: load results path
SO->>SP: OpenMCStatePoint(statefile, volfile)
SP->>CD: OpenMCCellData.from_files(volumes.json, geometry/materials XML)
CD-->>SP: {volumes, densities, masses}
loop per tally
RP->>RP: read mod option
alt option == VOLUME
RP->>MOD: volume(tally, volumes=CD.volumes(cells))
else option == MASS
RP->>MOD: mass(tally, masses=CD.masses(cells))
else other mods
RP->>MOD: apply other modifier(...)
end
end
sequenceDiagram
autonumber
participant SP as OpenMCStatePoint
participant CD as OpenMCCellData
participant JSON as volumes.json
participant XML as geometry/materials.xml
SP->>CD: from_files(JSON, XML)
CD->>JSON: from_json
JSON-->>CD: cell_volumes: dict
CD->>XML: from_xml
XML-->>CD: cell_densities: dict
CD->>CD: from_volume_density(volumes, densities) -> masses
CD-->>SP: OpenMCCellData(volumes, densities, masses)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/source/dev/insertbenchmarks.rst(1 hunks)src/jade/config/raw_config.py(1 hunks)src/jade/helper/openmc.py(8 hunks)src/jade/post/manipulate_tally.py(2 hunks)src/jade/post/raw_processor.py(2 hunks)src/jade/post/sim_output.py(3 hunks)src/jade/resources/default_cfg/benchmarks_pp/raw/openmc/Simple_Tokamak.yaml(2 hunks)tests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Simple_Tokamak/Simple_Tokamak/geometry.xml(1 hunks)tests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Simple_Tokamak/Simple_Tokamak/materials.xml(1 hunks)tests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Simple_Tokamak/Simple_Tokamak/settings.xml(1 hunks)tests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Simple_Tokamak/Simple_Tokamak/tallies.xml(1 hunks)tests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Simple_Tokamak/Simple_Tokamak/volumes.json(1 hunks)tests/helper/test_openmc.py(2 hunks)tests/post/test_manipulate_tally.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/jade/post/raw_processor.py (1)
src/jade/config/raw_config.py (2)
ConfigRawProcessor(11-34)TallyModOption(82-103)
src/jade/post/manipulate_tally.py (2)
src/jade/helper/openmc.py (2)
volumes(113-125)masses(141-151)src/jade/config/raw_config.py (1)
TallyModOption(82-103)
tests/helper/test_openmc.py (1)
src/jade/helper/openmc.py (4)
OpenMCCellData(29-151)from_files(47-55)OpenMCStatePoint(399-583)tallies_to_dataframes(561-583)
src/jade/post/sim_output.py (1)
src/jade/helper/openmc.py (1)
OpenMCStatePoint(399-583)
tests/post/test_manipulate_tally.py (2)
src/jade/post/manipulate_tally.py (2)
volume(324-352)mass(355-383)src/jade/helper/openmc.py (2)
volumes(113-125)masses(141-151)
🔇 Additional comments (11)
tests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Simple_Tokamak/Simple_Tokamak/settings.xml (1)
1-20: LGTM! Test fixture is well-formed.This settings.xml file provides a complete OpenMC fixed-source configuration for testing. The parameters (1000 particles, 10 batches, 14 MeV source energy) are appropriate for a fusion benchmark test case.
docs/source/dev/insertbenchmarks.rst (1)
172-173: LGTM! Documentation clearly describes new modifiers.The addition of
volumeandmassmodifiers is well-documented, specifying their data sources (volumes.json and OpenMC XML files) and that no arguments are required.tests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Simple_Tokamak/Simple_Tokamak/geometry.xml (1)
1-515: LGTM! Comprehensive geometry fixture.This geometry.xml provides a detailed tokamak geometry definition with multiple cells, materials, and surfaces. The structure is well-formed and appropriate for testing the OpenMC data loading pathway.
src/jade/resources/default_cfg/benchmarks_pp/raw/openmc/Simple_Tokamak.yaml (2)
56-121: Consistent application of volume modifier across flux/spectrum tallies.The two-step pattern (volume → replace) has been systematically applied to:
- Neutron/photon flux in blanket cells and PF coils
- Neutron/photon spectra in OB blanket and PF coils
- Tritium production in blanket
- DPA in divertor
This aligns with the PR objective to integrate volume divisors into the raw config YAML.
94-111: Verify nuclear heating tallies include volume normalization
Flux and spectrum blocks apply['volume', {}]before replacement, but heating blocks (lines 94–111) only use replace. If heating tallies should be per-unit-volume, add a volume step before replace.src/jade/post/sim_output.py (2)
284-286: LGTM! Correctly updated to remove tally factors file.The change from unpacking 4 values to 3 values, and passing only
statefileandvolfiletoOpenMCStatePoint, correctly implements the removal of the tally factors file path. This aligns with the PR objective.Based on the related code snippet from
src/jade/helper/openmc.py(lines 398-582), theOpenMCStatePoint.__init__signature now correctly accepts(spfile_path, volfile_path).
309-334: LGTM! Signature and implementation correctly updated.The
retrieve_filemethod has been properly updated to:
- Return 3 values instead of 4 (removed tffile)
- Assign
volumes.jsontofile3- Update return type annotation to
tuple[PathLike, PathLike, PathLike | None]This is consistent with the changes at the call site (line 284).
tests/dummy_structure/simulations/_openmc_-_FENDL 3.2b_/Simple_Tokamak/Simple_Tokamak/volumes.json (1)
1-49: LGTM! Valid volumes.json test fixture.This JSON file provides per-cell volume data for the Simple_Tokamak test case. The structure correctly maps cell IDs (as strings) to volume counts, which will be used by the new
OpenMCCellData.from_filesloading pathway.src/jade/config/raw_config.py (1)
102-103: LGTM! Enum members correctly added for new modifiers.The addition of
VOLUMEandMASSto theTallyModOptionenum is straightforward and follows the existing pattern. These values will be used by the tally modification functions inmanipulate_tally.py.tests/post/test_manipulate_tally.py (2)
21-22: LGTM! Imports added for new functions.
383-406: LGTM! Tests adequately cover new volume and mass functions.Both
test_volumeandtest_massverify that:
- The functions correctly divide
ValueandErrorcolumns by the per-cell divisor- The division is applied when a
Cellscolumn is present- Float arithmetic is handled correctly with appropriate tolerance
The tests provide good coverage of the basic functionality.
| if "Cells" in tally: | ||
| cells = tally.Cells.unique() | ||
| for cell in cells: | ||
| tally["Value"] = np.where( | ||
| (tally["Cells"] == cell), | ||
| tally["Value"] / volumes[cell], | ||
| tally["Value"], | ||
| ) | ||
| tally["Error"] = np.where( | ||
| (tally["Cells"] == cell), | ||
| tally["Error"] / volumes[cell], | ||
| tally["Error"], | ||
| ) | ||
| return tally |
There was a problem hiding this comment.
Keep relative errors untouched when applying volume/mass divisors
Throughout this module we treat "Error" as a relative quantity (e.g. condense_groups converts it to absolute via Error * Value, compare_data assumes the same). Dividing the relative error by a volume or mass here produces incorrect propagation and corrupts downstream aggregations. Only the value should be scaled; the relative error must remain unchanged (and we should fail fast if a requested cell has no divisor). Please adjust both helpers accordingly.
- if "Cells" in tally:
- cells = tally.Cells.unique()
- for cell in cells:
- tally["Value"] = np.where(
- (tally["Cells"] == cell),
- tally["Value"] / volumes[cell],
- tally["Value"],
- )
- tally["Error"] = np.where(
- (tally["Cells"] == cell),
- tally["Error"] / volumes[cell],
- tally["Error"],
- )
- return tally
+ if "Cells" not in tally:
+ return tally
+
+ for cell in tally["Cells"].unique():
+ if cell not in volumes:
+ raise KeyError(f"Volume not found for cell {cell}")
+ tally["Value"] = np.where(
+ tally["Cells"] == cell,
+ tally["Value"] / volumes[cell],
+ tally["Value"],
+ )
+ return tally
@@
- if "Cells" in tally:
- cells = tally.Cells.unique()
- for cell in cells:
- tally["Value"] = np.where(
- (tally["Cells"] == cell),
- tally["Value"] / masses[cell],
- tally["Value"],
- )
- tally["Error"] = np.where(
- (tally["Cells"] == cell),
- tally["Error"] / masses[cell],
- tally["Error"],
- )
- return tally
+ if "Cells" not in tally:
+ return tally
+
+ for cell in tally["Cells"].unique():
+ if cell not in masses:
+ raise KeyError(f"Mass not found for cell {cell}")
+ tally["Value"] = np.where(
+ tally["Cells"] == cell,
+ tally["Value"] / masses[cell],
+ tally["Value"],
+ )
+ return tallyAlso applies to: 370-383
🤖 Prompt for AI Agents
In src/jade/post/manipulate_tally.py around lines 339-352 (and similarly
370-383), the code wrongly divides the "Error" column by cell volume/mass even
though "Error" is a relative quantity; update the logic so only "Value" is
scaled by volumes[cell] while "Error" remains unchanged, and add a fail-fast
check that raises an informative exception if a referenced cell has no
corresponding divisor in volumes (do this before applying any scaling). Ensure
both helper sections are adjusted identically so downstream functions that
expect relative "Error" values remain correct.
There was a problem hiding this comment.
we can assume the volume and masses have no uncertainty, hence errors are not affected
There was a problem hiding this comment.
@dodu94, I appreciate the clarification about volumes and masses having no uncertainty. However, there's a subtlety in how relative errors propagate that affects the implementation.
Throughout this module, Error is treated as a relative error (σ/value), not an absolute error. We can see this in several places:
condense_groups(lines 71-72): converts to absolute error viaError * Value, then back to relative viaabs_err / Valuecompare_data(lines 514-517): usesv1 * e1to get absolute error, confirming Error is relativecumulative_sum(lines 283-286): explicitly converts between relative and absolute forms
When you divide a value by a constant with no uncertainty, the relative error remains unchanged:
Original: Value = V, Error = σ/V
After dividing by volume c: Value' = V/c, Error' should still = σ/V (not σ/(V·c))
The current code divides Error by the volume/mass, which would only be correct if Error were an absolute error. Since it's relative, this corrupts the error propagation.
Example:
- Original: Value = 100, Error = 0.05 (5% relative error, so σ = 5)
- After dividing by volume = 10:
- Correct: Value = 10, Error = 0.05 (still 5%, σ = 0.5)
- Current code: Value = 10, Error = 0.005 (0.5%, σ = 0.05) ❌
My original suggestion stands: only Value should be scaled; Error (being relative) should remain unchanged. This ensures consistency with the rest of the module's error propagation logic.
Would you like me to search for any additional evidence in the codebase that confirms this interpretation?
🧠 Learnings used
Learnt from: mcampos16
PR: JADE-V-V/JADE#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.
| * ``no_action``: no action is taken on the tally. No arguments are expected. | ||
| * ``volume``: a volume divisor is applied to the tally, obtained from a ``volumes.json`` file supplied with the bechmark inputs. No arguments are expected. | ||
| * ``mass``: a mass divisor is applied to the tally, obtained from a ``volumes.json`` file supplied with the bechmark inputs, and OpenMC ``xml`` files. No arguments are expected. | ||
| * ``scale``: the tally is scaled by a factor. The *factor* is expected as key argument and the provided value can |
There was a problem hiding this comment.
please specify that these modifiers can only be used in OpenMC
dodu94
left a comment
dodu94
left a comment
There was a problem hiding this comment.
doc comment was already pushed, thanks!
| if "Cells" in tally: | ||
| cells = tally.Cells.unique() | ||
| for cell in cells: | ||
| tally["Value"] = np.where( | ||
| (tally["Cells"] == cell), | ||
| tally["Value"] / volumes[cell], | ||
| tally["Value"], | ||
| ) | ||
| tally["Error"] = np.where( | ||
| (tally["Cells"] == cell), | ||
| tally["Error"] / volumes[cell], | ||
| tally["Error"], | ||
| ) | ||
| return tally |
There was a problem hiding this comment.
we can assume the volume and masses have no uncertainty, hence errors are not affected
Description
Due to duplication of scaling factors in the
tally_factors.yamlfile, and insrc/jade/default_cfg/benchmarks_pp/raw.yamlfiles, volume and mass divisors, often required to be applied to openmc results, have been moved across. This reduced the number of files required to be configured by the user when adding a new benchmark. Volumes are still obtained from avolumes.jsonfile, which is supplied alongside the OpenMC input files. Masses are obtained by extracting density values from OpenMCgeometry.xmlandmaterials.xmlfiles, however, these volume and mass divisors are now applied insrc/jade/post/manipulate_tally.py.Fixes #423
Type of change
Please select what type of change this is.
Other changes
Testing
Existing tests have been re-factored where
tally_factor.yamlfiles were required, or passed in as arguments to restore existing functionality. Numerical tests have been adjusted accordingly.tests/post/test_raw_processor.pyhave been updated to ensure volume and mass divisors are applied during raw processing for the OpenMC simple tokamak benchmark. In addition, new numerical tests have been added totests/post/test_manipulate_tally.py.Checklist:
Summary by CodeRabbit
New Features
Documentation
Refactor
Chores
Tests