Skip to content

Add random ray info to statepoint file #3499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

spasmann
Copy link
Contributor

@spasmann spasmann commented Jul 14, 2025

Description

This PR adds some of the basic random ray simulation parameters and summary information to the statepoint file. I tried to copy similar additions but please let me know if things could be organized differently/better!

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)

@spasmann spasmann requested a review from jtramm as a code owner July 14, 2025 01:49
@jtramm
Copy link
Contributor

jtramm commented Jul 14, 2025

This is great -- thanks so much @spasmann!

The only potential change that comes to mind might be to have the python statepoint object variables stored in a random_ray dictionary rather than at that top level, so as to mirror how the input settings are handled in the settings.random_ray dictionary. As the random ray implementation grows there are bound to be more settings introduced, so may be nice to keep them in their own dictionary.

If it's a huge pain to change (or you disagree with the dictionary idea in general), let me know! If it's just a matter of free time, let me know and I can give the conversion into a dictionary a go.

@spasmann
Copy link
Contributor Author

I think that's a good idea @jtramm, see if this looks better.

Copy link
Contributor

@jtramm jtramm left a comment

Choose a reason for hiding this comment

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

Thanks @spasmann - the new dictionary-based API looks great! Just a few minor comments below for your consideration.

Comment on lines +49 to +62
static double avg_miss_rate; // Average ray miss rate per
// iteration for reporting
static unique_ptr<Source> ray_source_; // Starting source for ray sampling
static RandomRaySourceShape source_shape_; // Flag for linear source
static bool mesh_subdivision_enabled_; // Flag for mesh subdivision
static RandomRaySampleMethod sample_method_; // Flag for sampling method
static int64_t n_source_regions; // Total number of source regions
static int64_t
n_external_source_regions; // Total number of source regions with
// non-zero external source terms
static uint64_t total_geometric_intersections; // Tracks the total number of
// geometric intersections by
// all rays for reporting
static int negroups_; // Number of energy groups
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The class variables should have a _ at the end to follow the C++ class variable naming convention.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a "total_geometric_intersections" variable already in the RandomRaySimulation class. Perhaps we should just use that one instead (and make it static), rather than making a duplicate?

{
negroups_ = data::mg.num_energy_groups_;
Copy link
Contributor

Choose a reason for hiding this comment

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

As negroups_ is now a shared global variable, all threads will be contending to write and rewrite to this variable in an uncontrolled manner in parallel. Perhaps this one can be left as a local variable? Then, we can just use the data::mg.num_energy_groups_ variable directly when it is needed for writing out to the statepoint, if that's possible. Longer term, it would be nice if all the random ray code just used one negroups variable somewhere, rather than each class having its own copy, but that is beyond the scope of this PR for sure.

Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

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

In general, I'm hoping to move away from using the statepoint/summary.h5 files to store information that is already found in the input XML file. That we write out input options to these files is a historical artifact from before we supported reading data out of XML files from the Python API. Ergo, if we are adding additional information in the statepoint file, I think it should be limited to actual results (average miss rate being a good example) rather than just things that were in the Settings object.

@jtramm
Copy link
Contributor

jtramm commented Jul 18, 2025

@paulromano - that is definitely a valid point. In general it is not good to store the same data twice if it can be avoided.

There is however the case where you are doing a parameter sweep or collecting data for a number of different setting options. We will naturally be saving the statepoints somewhere after each run so we can analyze them later (e.g., load tallies and see how they changed), but may neglect to save a copy of the input XML to go along with each unique statepoint. In that case, you wouldn't know which statepoint belonged to which input setting choice. The fix of course is to save both the input XML and the output statepoint for each case you're running, but it is sort of nice to have the statepoint be fully self descriptive. Removing input settings from the statepoint seems like it would unavoidably lead to some number of statepoints having to be recomputed as users forgot to also save the XML that corresponded to it.

@jtramm
Copy link
Contributor

jtramm commented Jul 18, 2025

Another potential issue I suppose is that if users are able to load some settings from the statepoint, they will likely assume that all the settings are in there as well, so may not bother to save the .xml that goes along with their statepoint. If we wanted to switch to storing settings in the .xml alone, might be better to make that jump all at once rather than migrating there over time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants