Skip to content

Conversation

@paulromano
Copy link
Contributor

A user reported that a model with periodic boundary conditions used to work in 0.12.0 and now doesn't work in 0.12.1. It turns out that translational periodic boundary conditions on general planes (Ax + By + Cz = D) only work right now if √(A² + B² + C²) = 1. This PR fixes this so that the normalization of the surface coefficients doesn't matter. I've added a simple regression test that will catch this case in the future.

@paulromano paulromano requested a review from smharper April 19, 2021 12:49
@paulromano paulromano force-pushed the translational-periodic-fix branch from ff6f3f8 to 407c93a Compare April 19, 2021 12:58
@paulromano paulromano force-pushed the translational-periodic-fix branch from 407c93a to 898f052 Compare April 21, 2021 12:24
@paulromano
Copy link
Contributor Author

I had to make a few more changes on this branch. First, I reverted the temporary hostname fix in #1816 since there is now an upstream fix on GitHub Actions VM images. I also discovered that the recent introduction of the libpnetcdf-dev package in our CI runs complicated the MPI setup. The problem can be summarized as follows:

  • libpnetcdf-dev depends on OpenMPI
  • Our current CI setup also installs MPICH, so then both OpenMPI and MPICH are installed
  • Ubuntu gives higher priority to OpenMPI, so when both are installed mpicc is a symbolic link to mpicc.openmpi
  • Almost everything then (mpicxx, mpiexec) used OpenMPI by default but we only had the libhdf5-mpich-dev package installed so that parallel HDF5 should have used MPICH
  • It looks like builds that should have been using parallel HDF5 actually weren't, instead falling back to the serial version

A slightly separate issue:

  • When we install h5py, it uses a wheel from PyPI and thus we don't build it against the same HDF5 library that is used to build OpenMC
  • This can create a version mismatch, e.g. wheel using 1.12 and package using 1.10, and unpredictable behavior when running OpenMC. If you call functions via openmc.lib but import h5py first, OpenMC will use the library that h5py links against. Otherwise it will use the version from apt.

Bottom line — my solution to both these issues is to 1) rely only on OpenMPI and 2) make sure h5py is built against whatever HDF5 library is used to build OpenMC. This uncovered a bug in the cpp_driver test whereby openmc_init was not getting called with the proper MPI communicator for parallel builds, so that is now fixed as well.

Copy link
Contributor

@smharper smharper left a comment

Choose a reason for hiding this comment

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

Ah, normal does not mean orthonormal as it turns out! Thanks for the fix. The changes to the code look good, and the additional test is nice.

@smharper smharper merged commit efcc0c1 into openmc-dev:develop Apr 22, 2021
@smharper
Copy link
Contributor

I've run into the HDF5 version mismatch before. I wonder if it makes sense for us to essentially roll our own h5py to ensure that the transport solver and the Python API are both using the same HDF5 libs. We already hide the h5py calls so a switch would hopefully be seamless for our users, and we can probably reuse a lot of hdf5_interface.cpp code.

@paulromano paulromano deleted the translational-periodic-fix branch April 23, 2021 12:24
pshriwise pushed a commit to pshriwise/openmc that referenced this pull request Apr 24, 2021
…odic-fix

Fix for translational periodic boundary conditions
pshriwise pushed a commit to pshriwise/openmc that referenced this pull request Apr 24, 2021
…odic-fix

Fix for translational periodic boundary conditions
pshriwise pushed a commit to pshriwise/openmc that referenced this pull request Apr 24, 2021
…odic-fix

Fix for translational periodic boundary conditions
pshriwise pushed a commit to pshriwise/openmc that referenced this pull request Apr 26, 2021
…odic-fix

Fix for translational periodic boundary conditions
paulromano pushed a commit that referenced this pull request Apr 26, 2021
Fix for translational periodic boundary conditions
@paulromano paulromano mentioned this pull request Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants