Update openmc

[aprilnovak]

No description provided.

[moosebuild]

Job Documentation on 9869103 wanted to post the following:

View the site here

This comment will be updated on new commits.

[shikhar413]

@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 openmc-dev/openmc#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 openmc-dev/openmc#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]

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).

[shikhar413]

Hi @aprilnovak I noticed that the OpenMC version was updated recently in #983, should this PR be closed in favor of the other one?

[aprilnovak]

Yes I think this one can be closed -- ultimately, to update OpenMC we didn't need to also add anything special about the source sampling sites. Maybe that means that the newest version of OpenMC fixes the behavior that @shikhar413 was originally updating? (also I am not sure if I'm misunderstanding, but the commits on this branch don't match what I expect them to, given the context from the discussion).

[shikhar413]

Yeah, honestly quite surprised you were able to get all tests to pass in the other PR, given that there were many tests failures happening here. For my own use cases, having Cardinal point to a newer OpenMC version was all I need so I'm fine closing this PR. If similar issues with test faiulres occur later on, let me know and I can try to take a look