Skip to content

Add stat:sum field to MCPL files for proper weight normalization - #3522

Merged
paulromano merged 8 commits into
openmc-dev:developfrom
bpolania:fix-mcpl-stat-sum
Aug 8, 2025
Merged

Add stat:sum field to MCPL files for proper weight normalization#3522
paulromano merged 8 commits into
openmc-dev:developfrom
bpolania:fix-mcpl-stat-sum

Conversation

@bpolania

@bpolania bpolania commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

Description

This PR implements the stat:sum field in MCPL file headers to enable proper weight normalization when merging MCPL files, as requested in #3514.

Changes

  • Adds stat:sum field (key: "openmc_np1") to MCPL file headers for files written with MCPL >= 2.1.0
  • Implements crash-safety pattern: initially sets to -1, updates with actual particle count before closing
  • Maintains backward compatibility with MCPL < 2.1.0 (gracefully degrades if function not available)
  • Adds comprehensive unit tests in both C++ and Python

Implementation Details

The implementation follows the specification from @tkittel:

  • Uses mcpl_hdr_add_stat_sum() function from MCPL >= 2.1.0
  • Key name: "openmc_np1"
  • Initial value: -1 (indicates incomplete file if creation is interrupted)
  • Final value: total number of source particles in the simulation

Testing

  • C++ unit tests added (tests/cpp_unit_tests/test_mcpl_stat_sum.cpp)
  • Python unit tests added (tests/unit_tests/test_mcpl_stat_sum.py)
  • All existing tests pass
  • Code formatted with clang-format via commit hooks

Fixes

Fixes #3514

Checklist

  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have run clang-format on C++ code via the commit hooks

@bpolania
Add stat:sum field to MCPL files for proper weight normalization (ope…
6979b4b 
…nmc-dev#3514)

- Implements stat:sum field (key: "openmc_np1") in MCPL file headers
- Initially sets to -1 for crash safety, updates with particle count before closing
- Compatible with MCPL >= 2.1.0, gracefully degrades for older versions
- Enables proper file merging and McStas/McXtrace integration
- Adds C++ and Python unit tests
@bpolania
bpolania requested a review from ebknudsen as a code owner August 6, 2025 06:14
@tkittel

tkittel commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

Thanks a lot for picking up this work @bpolania, it is truly appreciated! :-)

It looks good to me in general, but I have a few comments and questions. Some I will leave in-line as a review, but two things I would like to mention in particular here (also, @ebknudsen might have some comments):

  1. The first is more for the OpenMC core dev team(@paulromano et al): The choice of key "openmc_np1" is something that we will be stuck with going forward, so I think it is important if core OpenMC devs chime in on whether they are happy with that. In particular I am wondering if "np1" is normal terminology in the context of OpenMC, or was it just chosen to mimic my choice of ssw_np1 for the MCNP converters? If "np1" is not normal OpenMC terminology, is there something more appropriate? Like "openmc_src_count" or "openmc_src_nparticles"? Or we could take this opportunity to decide on some sort of community standard (which I could then also add to the ssw2mcpl converter for MCNP), like "src_nparticles" or mc_src_count or whatever? That would make common tooling around these files easier.
  2. I am not exactly sure how MPI and bank_index are used in the implementation. But of course, I wanted to double-check that:
    • The value written into the new stat:sum entry is the value of the number of source particles, not the number of particles written to the MCPL file. In other words, if running a simulation with 1e6 primary particles, but only 117 particles are written to the MCPL file (those particles, primary or secondary, hitting some surface or whatnot), then the new stat:sum value written should be 1e6, not 117 (the latter is anyway available as the number of particles contained in the MCPL file).
    • If each MPI process writes its own file, the value written to the stat:sum entry should reflect the number of source particles used in that process, not the global number.

@tkittel
tkittel self-requested a review August 6, 2025 07:08
tkittel
tkittel reviewed Aug 6, 2025
View reviewed changes
Comment thread src/mcpl_interface.cpp Outdated
Comment thread src/mcpl_interface.cpp Outdated
Comment thread src/mcpl_interface.cpp Outdated
Comment thread tests/unit_tests/test_mcpl_stat_sum.py Outdated
Comment thread tests/unit_tests/test_mcpl_stat_sum.py Outdated
bpolania added 2 commits August 6, 2025 00:39
@bpolania
Address PR review comments
787ba30 
- Remove unused mcpl_hdr_add_data_fpt declaration and related code
- Fix empty bank_index case to return -1 instead of 0 (statistics unavailable)
- Update Python test to use hasattr(f,'stat_sum') convenience property
- Clarify that mcpl_hdr_add_data was available from MCPL beginning, not 2.1.0
@bpolania
Fix clang-format version 15 compatibility
58b1c92 
Adjust line break in function pointer declaration to match clang-format v15 style used by CI
@bpolania

bpolania commented Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

Thank you @tkittel for the review! I've addressed all your feedback in commits 787ba30 and 58b1c92:

Changes made:

  • Removed the unused mcpl_hdr_add_data_fpt declaration and all related code since it wasn't being used
  • Fixed the empty bank_index case to return -1 (statistics unavailable) instead of 0, maintaining consistency with the crash-safety pattern
  • Updated the Python test to use hasattr(f, 'stat_sum') and access the .stat_sum property directly when available, with a fallback to parsing comments for older MCPL versions
  • Removed the unnecessary hasattr check for .comments since it's guaranteed to exist
  • Fixed clang-format compatibility with version 15 used by the CI

@tkittel

tkittel commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

Thanks @bpolania ! So from my POV, all that is missing is feedback (and perhaps a general review) from the core devs about the items I raised in my previous comment :-)

bpolania added 3 commits August 6, 2025 01:39
@bpolania
Fix Python test to work with actual OpenMC simulation
e86a06f 
- Use openmc.examples.pwr_pin_cell() which includes cross sections
- Run full eigenvalue simulation to generate MCPL files
- Handle cases where MCPL interface or files are not available
- Test uses proper sourcepoint setting and batch configuration
@bpolania
Fix stat:sum to use total source particles, not file particle count
a255618 
Per issue openmc-dev#3514 requirements, stat:sum should contain the original number
of source particles in the simulation, not the number written to the file.

Changed implementation to calculate:
  (n_batches - n_inactive) * n_particles
instead of using bank_index.back()

This ensures proper weight normalization when merging MCPL files.
@bpolania
Apply clang-format formatting
2a93f20
@bpolania

bpolania commented Aug 6, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @bpolania ! So from my POV, all that is missing is feedback (and perhaps a general review) from the core devs about the items I raised in my previous comment :-)

Well, still failing some tests. I just pushed again and see if they are fixed.

@yrrepy

yrrepy commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

I like:

  • openmc_src_nparticles
  • src_nparticles

or

  • ssw_src_nparticles
  • ssw_src_particles

paulromano
paulromano approved these changes Aug 8, 2025
View reviewed changes

@paulromano paulromano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bpolania and @tkittel! I made a few small updates, notably accounting for settings::gen_per_batch in the total number of particles. Good to go from my end!

@paulromano
paulromano enabled auto-merge (squash) August 8, 2025 10:49
@paulromano
paulromano merged commit e36c0ae into openmc-dev:develop Aug 8, 2025
apingegno pushed a commit to apingegno/openmc that referenced this pull request May 7, 2026
@bpolania @paulromano
Add stat:sum field to MCPL files for proper weight normalization (ope…
d120c0f 
…nmc-dev#3522)

Co-authored-by: Paul Romano <paul.k.romano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tkittel tkittel tkittel left review comments
@paulromano paulromano paulromano approved these changes
@ebknudsen ebknudsen Awaiting requested review from ebknudsen ebknudsen is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MCPL integration in OpenMC

4 participants

@bpolania @tkittel @yrrepy @paulromano