Activated thermal option in CO2-brine fluid model

[francoishamon]

This PR enables the thermal option in the CO2-brine fluid model by activating the computation of the phase internal energy and phase enthalpy. This is the last hurdle before we can test the new thermal assembly kernels implemented in #1225.

Precisely, this PR:

• Adds the phase internal energy and enthalpy fields in MultiFluidBase.
• Renames MultiPhaseMultiComponentFluid into CO2BrineFluid because this class is really specific to the two-phase two-component CO2-brine system. It makes the GitHub diffs a little bit difficult to read, sorry.
• Adds a PhaseModel to group submodels for density, viscosity, internal energy, and enthalpy.
• Refactors the CO2-brine class, now templated on PHASEMODEL1, PHASEMODEL2, and FLASH

Will require a rebaseline, done in https://github.com/GEOSX/integratedTests/pull/196

[klevzoff]

LGTM, mainly just have a suggestion to group PVT property types and reduce the number of template parameters (see details in the comment).

Also, I think you will still need a rebaseline even for stuff not written into restart (similar to one you've had to do recently).

[klevzoff]

Looking at this, I'm beginning to think we should once again try simple structs to reduce the boilerplate. E.g. (code is approximate and not verified):

template< typename DENS, typename VISC, typename ENTH, typename INTENERGY >
struct PhaseModel
{
  using Density = DENS;
  using Viscosity = VISC;
  using Enthalpy = ENTH;
  using InternalEnergy = INTENERGY;

  Density density;
  Viscosity viscosity;
  Enthalpy enthalpy;
  InternalEnergy internalEnergy; // I wish there was a short one-word term for this

  struct KernelWrapper
  {
    Density::KernelWrapper density;
    Viscosity::KernelWrapper viscosity;
    Enthalpy::KernelWrapper enthalpy;
    InternalEnergy::KernelWrapper internalEnergy;
  };
};

Then the fluid class becomes

Suggested change

	template< typename P1DENS, typename P1VISC, typename P1ENTH, typename P1INTENERGY,
	typename P2DENS, typename P2VISC, typename P2ENTH, typename P2INTENERGY,
	typename FLASH >
	class CO2BrineFluid : public MultiFluidBase
	template< typename PHASE1, typename PHASE2, typename FLASH >
	class CO2BrineFluid : public MultiFluidBase

and further on we replace all of

std::unique_ptr< P1DENS > m_p1Density;
std::unique_ptr< P1VISC > m_p1Viscosity;
...

with just a single

std::unique_ptr< PHASE1 > m_phase1;

and similarly in the wrapper you get

PHASE1::KernelWrapper m_phase1;

and kernel wrapper constructor argument list can be reduced accordingly.

@francoishamon You probably weren't planning on embarking on another refactoring journey in this PR, my apologies for throwing more work your way. As always, it's just a suggestion, only take it if you like it and have time to implement. I wish I had this idea back when you first refactored this class for GPU and we introduced property models as template parameters; adding new properties would've involved less change.

[TotoGaz]

I'm beginning to think we should once again try simple structs to reduce the boilerplate [...] You probably weren't planning on embarking on another refactoring journey in this PR.

Yesterday it was 36 parameters functions or six dimensional arrays.
Today it's a 9 template parameters class.
Refactoring is faaaar too complicated (I've experienced it).
Things need to be done (like #1709 or #1750), and it shall take a significant amount of our time.
Thank God we have a solid testing policy! 👍

[francoishamon]

Suggestion highly appreciated @klevzoff! I implemented it, and from what I see so far on Lassen, it fixes the gcc compilation error (formal parameter space overflow) that was the pebble in my shoe for weeks in the thermal PR (see for instance here). I may be wrong, so I will push soon to confirm this with the Travis CI tests. Thanks!

[francoishamon]

I implemented what you suggested in a separate file called PhaseModel.hpp and reduced the number of templates in the CO2-brine fluid class. It is much cleaner now, thanks for your suggestion.

However, I rejoiced too quickly regarding my longstanding compilation error, it is still present in the fluid update kernel (but just does not appear on Lassen with gcc or clang). The error appears when the update kernel is called from PVTDriver as shown in the logs, but also when the update kernel is called from the solver:

***/GEOSX_TPL-a634fc6/raja/include/RAJA/policy/cuda/forall.hpp(137): Error: Formal parameter space overflowed (4408 bytes required, max 4096 bytes allowed) in function _ZN4RAJA6policy4cuda4impl18forall_cuda_kernelILm256ENS_9Iterators16numeric_iteratorIllPlEEZN5geosx34CompositionalMultiphaseBaseKernels17FluidUpdateKernel6launchINS1_9cuda_execILm256ELb0EEENS8_12constitutive13CO2BrineFluidINSE_10PhaseModelINSE_8PVTProps19EzrokhiBrineDensityENSH_21EzrokhiBrineViscosityENSH_15NoOpPVTFunctionESK_EENSG_INSH_20SpanWagnerCO2DensityENSH_20FenghourCO2ViscosityESK_SK_EENSH_13CO2SolubilityEE13KernelWrapperEEEvlRKT0_RKN7LvArray9ArrayViewIKdLi1ELi0ElNSV_10ChaiBufferEEES11_S11_RKNSW_ISX_Li2ELi0ElSY_EEEUllE_lEEvT1_SS_T2_

To overcome this issue, the only technique I am aware of would be to go back to the size-1 views:

arrayView1d< typename PHASE1::KernelWrapper > m_phase1;
arrayView1d< typename PHASE2::KernelWrapper > m_phase2;

instead of what I implemented here:
https://github.com/GEOSX/GEOSX/blob/ccb0a2e1aa2a95fc4eef5b22fbe7f287eb00cc6c/src/coreComponents/constitutive/fluid/CO2BrineFluid.hpp#L135-L139
but this is really ugly. Is there a better way to do this? In this discussion, using a unique_ptr was mentioned, would it be cleaner?

[klevzoff]

@francoishamon I actually wrote a response to you weeks ago on the energy balance PR, but I accidentally added it as a review comment, so it was left unpublished :( I've posted it now #1225 (comment), it's not a definitive answer but some ideas about reducing the size of arguments.

[francoishamon]

@klevzoff Thanks, it worked, this PR compiles now, this is great.

[klevzoff]

On a related note, I was really hoping that someone would eventually refactor this in a truly multi-phase multi-component model (using an ungodly amount of std::tuple or camp::tuple magic). The name change kind of gets in the way of that dream... but I don't oppose it if, for the moment, it makes things clearer. This new proper-multi-fluid class, if one ever sees the light of day and is considered to be useful, can be written separately, or the name change can be reversed later.