illinois-ceesd / mirgecom

MIRGE-Com is the workhorse simulation application for the Center for Exascale-Enabled Scramjet Design at the University of Illinois.
Other
12 stars 19 forks source link

Refactor boundary treatments #576

Open MTCam opened 2 years ago

MTCam commented 2 years ago

The current MIRGE-Com fluid boundary condition infrastructure is messy and hard to follow. We want to refactor it.

The general strategy may be to use inheritance properly and eliminate the need for the derived classes (bc's) to specify a collection of functions to customize the boundary treatment. Instead, function overrides can be used to clean this up. At the same time, we should comb through to make sure the operators and their use of the boundaries are properly passing through the numerical flux functions specified at the simulation driver level, and that the boundary things are using it properly.

majosm commented 2 years ago

One thing I think I'd like is if every BC explicitly implemented the flux functions that it's compatible with (using helper functions inside as needed to reduce code duplication), rather than just passing callbacks to the base class. This would prevent the possibility of, say, accidentally omitting a boundary callback when calling the base class constructor, and then having it silently apply an incorrect BC (which I have done a couple of times so far with the wall model BCs). Failing to implement a flux function in this explicit approach would instead result in a user-visible exception when the operator attempts to call it.

MTCam commented 2 years ago

One thing I think I'd like is if every BC explicitly implemented the flux functions that it's compatible with (using helper functions inside as needed to reduce code duplication), rather than just passing callbacks to the base class. This would prevent the possibility of, say, accidentally omitting a boundary callback when calling the base class constructor, and then having it silently apply an incorrect BC (which I have done a couple of times so far with the wall model BCs). Failing to implement a flux function in this explicit approach would instead result in a user-visible exception when the operator attempts to call it.

I like this, and agree, although I don't like it when every BC implements giant wads of repeated code. Trying to factor out all the repeated (error-prone) code was the main motivation for the boundary objects' current shape. The execution wasn't great - but can be made way better now.