Skip to content
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

Adding set_strides() to openmc_simulation_init() #2183

Merged
merged 2 commits into from
Aug 25, 2022

Conversation

lewisgross1296
Copy link
Contributor

Closes #2181. For applications that change their mesh between OpenMC runs (e.g. AMR in Cardinal), it will be important to update the bins for the estimators, as their structure will have changed. When the bins change, set_strides() needs to be called again to make sure the xtensor multidimensional results array is sized properly to collect tallies during the next Monte Carlo simulation.

Originally, the thought was to move set_strides() away from set_filters(), but this caused errors in local tests. It will potentially add duplicate, unnecessary computations if set_strides() is called twice, but the filters and mesh are the same. This occurs before the histories start though and is not expected to cause a significant slowdown.

It is possible, in theory, to refactor such that set_strides() is only called when the mesh/filters change, but the effort to get this right may not be worth the hassle. Especially when the simpler alternative isn't expected to concerningly degrade performance.

… fine to add in openmc_simulation_init() and in there should occur before init_results. this is because init_results use n_filter_bins_ which is modified by set_strides()
@lewisgross1296
Copy link
Contributor Author

Open to feedback about how else to do this, but the thought is that since init_results() uses n_filter_bins_ and set_strides() updates n_filter_bins_, calling this in openmc_simulation_init() right before should handle the changed mesh, while also leaving set_filters() as is and preventing issues from removing it there.

@paulromano
Copy link
Contributor

Originally, the thought was to move set_strides() away from set_filters(), but this caused errors in local tests.

I just tried doing the same thing locally and it doesn't seem to cause any problems on my end. What tests are you seeing problems in with this approach?

@lewisgross1296
Copy link
Contributor Author

Patrick and I realized that even my local tests were failing when I was using a clean copy of openmc/develop. I need to resolve this and try again. Though good to know yours works

…ird locally and this was not causing them to fail
@lewisgross1296
Copy link
Contributor Author

I think this failure is due to GitHub actions?

Run $GITHUB_WORKSPACE/tools/ci/gha-before-script.sh
+ source tools/ci/download-xs.sh
++ set -ex
++ [[ ! -e /home/runner/nndc_hdf5/cross_sections.xml ]]
++ tar -C /home/runner -xJ
++ wget -q -O - https://anl.box.com/shared/static/teaup95cqv8s9nn56hfn7ku8mmelr95p.xz
xz: (stdin): File format not recognized
tar: Child returned status 1
tar: Error is not recoverable: exiting now
Error: Process completed with exit code 2.

@pshriwise
Copy link
Contributor

I think this failure is due to GitHub actions?

Yeah it looks like it failed to download the cross section data. This happens sometimes. Restarting that job...

@paulromano paulromano merged commit ead6159 into openmc-dev:develop Aug 25, 2022
@lewisgross1296 lewisgross1296 deleted the strides_move branch August 25, 2022 14:42
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.

Move set_strides() from set_filters() to openmc_simulation_init()
3 participants