Skip to content

Conversation

@jtramm
Copy link
Contributor

@jtramm jtramm commented Oct 24, 2025

Description

Currently WeightWindowGenerator objects default to having a setting of max_realizations of 1. This is an important setting for MAGIC as users may only want to continue to update weight windows in the first few batches of a simulation.

However, for FW-CADIS generation with random ray, we will always just want to update weight windows once at the end of the simulation. Currently, this requires the user to set the max_realizations parameter to the number of batches. This PR simply instructs the random ray solver to ignore the max_realizations and update_interval logic. Some of the logic for when to update weight windows was also moved out of simulation.cpp and into the weight_windows.cpp file so that the solver type didn't need to be checked in two different places.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the weight window update logic to handle FW-CADIS generation with random ray mode differently from Monte Carlo mode. In random ray simulations, weight windows should only be updated once at the end of all batches, regardless of the max_realizations and update_interval settings.

Key changes:

  • Moved solver-specific weight window update logic from simulation.cpp into WeightWindowsGenerator::update() method
  • Random ray mode now updates weight windows only on the final batch, bypassing max_realizations and update_interval checks
  • Documentation updated to reflect that max_realizations parameter is no longer needed for FW-CADIS with random ray

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/weight_windows.cpp Added solver-type conditional logic to update weight windows only on final batch for random ray mode
src/simulation.cpp Removed outer conditional check that restricted weight window updates in random ray mode
docs/source/usersguide/variance_reduction.rst Removed max_realizations parameter from example code since it's now unnecessary for random ray mode

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@shimwell
Copy link
Member

Thanks so much for adding this nice user experience upgrades.

Would it be worth adding a warning if the user sets either max_realizations or update_interval just to print to terminal saying something like

"when using Random Ray mode to generate weight windows max_realizations / update_interval are ignored"

@jtramm
Copy link
Contributor Author

jtramm commented Oct 24, 2025

I'd be on board for the warning usually, but it looks like there is no default on the C++ side, it is a required XML tag:

WeightWindowsGenerator::WeightWindowsGenerator(pugi::xml_node node)
{
  // read information from the XML node
  int32_t mesh_id = std::stoi(get_node_value(node, "mesh"));
  int32_t mesh_idx = model::mesh_map[mesh_id];
  max_realizations_ = std::stoi(get_node_value(node, "max_realizations"));

-- the default of "1" gets set on the python side, as far as I can see:

def __init__(
       self,
       mesh: openmc.MeshBase,
       energy_bounds: Sequence[float] | None = None,
       particle_type: str = 'neutron',
       method: str = 'magic',
       max_realizations: int = 1,
       update_interval: int = 1,
       on_the_fly: bool = True
   ):

Thus, at runtime, OpenMC doesn't know if the user actually selected "1" or if that was just the default, so may not be worthy of a warning. We could also hypothetically have it warn the user on the python side about this, but that gets more complex as you have to account for the different ways an object can be initialized. It would be possible, but as there isn't a "failure mode" I can see if a user is ignorant of this setting now (as weight windows will now always be output at the appropriate time), it may not be worth it. If you think it would be really useful though, perhaps we can make an attempt to edit the python side, or change the XML setup to make that an optional XML tag and not have python output a default value.

@shimwell
Copy link
Member

fair enough, thanks for considering it. No need for the warning idea then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants