-
Notifications
You must be signed in to change notification settings - Fork 580
Random Ray Normalization Improvements #3051
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
Conversation
…rs change slightly due to the stochastic volume of each cell now being considered as part of normalization
yardasol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @jtramm, I have a couple suggestions to variable names and a function docstring. This is a good improvement to the fixed source random ray capabilities.
| By default, the unnormalized flux values (units of cm) will be reported. If the | ||
| user wishes to received volume normalized flux tallies, then an option for this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just for consistency with the way OpenMC reports flux tallies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was my thinking. I'm definitely open to switching to reporting volume normalized results by default (as that is probably the more natural way of reporting results for this solver). However, my thought was that people may be commonly comparing against the MGMC results, so may be useful to have it report apples-to-apples numbers by default to avoid confusion. We can always add some warnings through in the docs about this to highlight the difference. Anyway, I'm curious to hear what people's vote for the default should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way you have done it makes sense, and it's nice there is an option to normalize it. I definitely think adding a warning in the docs is a good addition if we end up keeping the normalized values as the default output.
| for (int e = 0; e < negroups_; e++) { | ||
| // Temperature and angle indices, if using multiple temperature | ||
| // data sets and/or anisotropic data sets. | ||
| // TODO: Currently assumes we are only using single temp/single | ||
| // angle data. | ||
| const int t = 0; | ||
| const int a = 0; | ||
| float sigma_t = data::mg.macro_xs_[material].get_xs( | ||
| MgxsType::TOTAL, e, nullptr, nullptr, nullptr, t, a); | ||
| simulation_external_source_strength += | ||
| external_source_[sr * negroups_ + e] * sigma_t * volume; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using sr as the index variable for source regions, I think it makes sense to use eg as the index variable for energy group
| for (int e = 0; e < negroups_; e++) { | |
| // Temperature and angle indices, if using multiple temperature | |
| // data sets and/or anisotropic data sets. | |
| // TODO: Currently assumes we are only using single temp/single | |
| // angle data. | |
| const int t = 0; | |
| const int a = 0; | |
| float sigma_t = data::mg.macro_xs_[material].get_xs( | |
| MgxsType::TOTAL, e, nullptr, nullptr, nullptr, t, a); | |
| simulation_external_source_strength += | |
| external_source_[sr * negroups_ + e] * sigma_t * volume; | |
| for (int eg = 0; eg < negroups_; eg++) { | |
| // Temperature and angle indices, if using multiple temperature | |
| // data sets and/or anisotropic data sets. | |
| // TODO: Currently assumes we are only using single temp/single | |
| // angle data. | |
| const int t = 0; | |
| const int a = 0; | |
| float sigma_t = data::mg.macro_xs_[material].get_xs( | |
| MgxsType::TOTAL, eg, nullptr, nullptr, nullptr, t, a); | |
| simulation_external_source_strength += | |
| external_source_[sr * negroups_ + eg] * sigma_t * volume; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This brings up a good point! As it is in the random ray solver, we currently have e and g used as iterators over energy groups. The eg idea is also reasonable. I think it would be best though to pick one and be consistent with it throughout random ray source files in OpenMC, and to add it to the source style guide at https://docs.openmc.org/en/stable/devguide/styleguide.html#naming
My vote would be to use g consistently for these loops, as the MGMC portion of the code is already using gin and gout for incoming/outgoing energy groups. Additionally, g matches the verbiage in all the random ray equations in the docs, which would be more of a pain to refactor as compared to the source code itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose that the variable names be left as they are in this PR, and I can commit to a separate PR that does the refactor on energy group loop variables for consistency throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eg is the mots clear to me, but g also makes sense. I agree, we should pick one and make it consistent across the whole codebase and add it to the source style guide. Doing all of that in a separate PR would be great!
| for (int g = 0; g < negroups_; g++) { | ||
| int idx = sr * negroups_ + g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency
| for (int g = 0; g < negroups_; g++) { | |
| int idx = sr * negroups_ + g; | |
| for (int eg = 0; eg < negroups_; eg++) { | |
| int idx = sr * negroups_ + eg; |
Co-authored-by: Olek <[email protected]>
yardasol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work here @jtramm. Should we wait for @paulromano's input before merging?
|
Thanks for your review @yardasol! If you're happy with the PR, I think you can merge once the tests have passed. |
Co-authored-by: Olek <[email protected]>
This PR improves the normalization of results in OpenMC in two small ways that should make it much easier to compare fixed source results in an "apples to apples" manner with Monte Carlo.
External Source Strength Normalization
The first enhancement is that when in fixed source mode, the tallies are now correctly normalized against the user inputted source strength. Previously, as volumetric sources could be applied to multiple source regions, the effective source strength may be many times higher/lower than the user specified total value. In Monte Carlo, this problem is not present, as particles are simply sampled from the source distribution regardless of its volume. In random ray, however, the volumetric source distribution must be applied to each FSR. As the volume of the FSR is not known apriori, then there is no way to know how much of the source should be allocated to each FSR. Thus, we were previously just copying the source to all FSRs, which may change the overall effective strength.
This PR fixes this issue by comparing the total user input source strength to the FSR-wise external source strength during tallying and plotting and computing a correction factor for the overall flux. This correction factor has to be updated at each iteration as it is based on the volume of each FSR, which improves over the course of the simulation.
Making Volume Normalization of Flux Tallies Optional
The second enhancement is that the user can now manually specify whether or not they want to have flux tallies be reported in units of cm (i.e., basically the total particle tracklength in cm in that spatial tally region) or in units of cm^-2 (i.e., the volumetric flux rate). Previously we had always been reporting the volume normalized value in units of cm^-2. In this PR, we also introduce an option:
that enables/disables this normalization. By default, it is set to
False, such that the default flux tallies that the random ray solver produces are apples-to-apples comparable to the default tallies that the Monte Carlo solver produces. This is a change in behavior vs. the existing method where volume normalized flux tallies are default, but in the long term this change seems like it will make it easier for users to compare to MC.Validation
For testing, I have implemented a simple 3 region cube problem with tallies of different spatial sizes, as shown below:
With volume normalization enabled, the results match Monte Carlo if normalized by the analytical volumes of each tally region.
Note that there is more error than we'd like to see in the source region, which features a void-like material and an external source. This will be the topic of a forthcoming PR on Issue #2990 that introduces additional volume estimators which will fix this sort of error. For now, it is clear the normalization process is clearly working as expected.
Note: The added python test has (currently) unused
@pytest.mark.parametrizehooks for testing alternative volume estimators, which will make re-use of this test in an upcoming PR.Checklist