neams-th-coe / cardinal

High-Fidelity Multiphysics
https://cardinal.cels.anl.gov/
Other
84 stars 39 forks source link

Update openmc #903

Open aprilnovak opened 1 month ago

moosebuild commented 1 month ago

Job Documentation on 55007f6 wanted to post the following:

View the site here

This comment will be updated on new commits.

shikhar413 commented 2 weeks ago

@aprilnovak sorry to bother you over the holiday break but I thought I'd leave this message here since I will also be taking a few days off next week, and wanted to give you an update on these test failures, since I was hitting similar errors with the source rejection error for some of my Cardinal models.

Basically, what was done previously before the update in https://github.com/openmc-dev/openmc/pull/2916 is that the number of rejected particles would be compared to the number of accepted particles when sampling only fissionable sites, and if this fraction was above some threshold, then the error for source site sampling was triggered. This check is done in source.cpp::IndependentSource::sample. However, if you look at this method, you will see that the variable n_reject is not defined as static whereas n_accept is defined as static, and so the fraction of rejected source sites was actually the number of rejected particles for that particular source site divided by the number of cumulative accepted particles, and so this error message was never triggered since it doesn't account for the cumulative number of rejected particles over all source sites. However, this was fixed in https://github.com/openmc-dev/openmc/pull/2916, where both n_accept and n_reject are now both static variables (i.e. the cumulative rejected particles are divided by the cumulative accepted particles), and some of your Cardinal tests are now triggering the threshold rejection particle fraction error message, possibly because the domain over which the source distribution bounding box Is defined encompasses a lot more non-fissionable material relative to the fissionable material.

The fix here for the Cardinal test failures is to reduce the domain over which to sample the source distribution to the minimum bounding box that covers the fissionable area of the geometry (this will probably be tedious to do for larger problems). Trying to expand the domain of the source distribution to include the entire geometry will no longer work if the geometry has a lot of non-fissionable material relative to the fissionable material, and the 95% source distribution error will likely be met. This was the case of for my model, just wanted to share this info in case you haven't got to the bottom of these test errors yet. Just a heads up, there might be other reasons for the test failures too, I haven't check all the test failures in detail

aprilnovak commented 1 week ago

Thanks @shikhar413 that is really helpful! I know that I need to update the files to use the new constraints syntax, and will be sure to use a smaller bounding box (or perhaps a domains specification for the source sites will also help).