-
Notifications
You must be signed in to change notification settings - Fork 89
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
refactor: Add component selector for fluid models #3213
refactor: Add component selector for fluid models #3213
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3213 +/- ##
===========================================
+ Coverage 55.71% 55.83% +0.11%
===========================================
Files 1041 1042 +1
Lines 88575 88652 +77
===========================================
+ Hits 49353 49501 +148
+ Misses 39222 39151 -71 ☔ View full report in Codecov by Sentry. |
template<> | ||
struct Components< CompositionalTwoPhaseConstantViscosity > | ||
{ | ||
using type = camp::int_seq< integer, 2, 3, 4, 5 >; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that you can do something like this to avoid specifying each number:
template<integer Begin, integer End, integer... Is>
struct create_integer_sequence_impl
: create_integer_sequence_impl<Begin, End - 1, End - 1, Is...> {};
template<integer Begin, Integer... Is>
struct create_integer_sequence_impl<Begin, Begin, Is...>
{
using type = camp::int_seq<integer, Is...>;
};
and then use it like:
template<>
struct Components< CompositionalTwoPhaseConstantViscosity >
{
using type = typename create_integer_sequence_impl<2, 5>::type;
}
and maybe I would define the value in the max and min value of components as constexpr in each fluid model itself and have something like:
template<typename FLUID_TYPE>
struct Components< >
{
using type = typename create_integer_sequence_impl<FLUID_TYPE::min_n_components, FLUID_TYPE::max_n_components>::type;
}
@wrtobin thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC camp has builtin support for range generation... but looking at it it seems like they only support [0...N) generation.
So yeah we should be able to avoid explicitly listing all numbers unless there is a compelling reason to do so.
I think its typically logical to add a third component for the step size which defaults to 1, so we can e.g. count by 2's etc, but that is just as a general mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we make it even more generic it could probably even go in common
or in datatypes
. Honestly, it should probably live directly in camp
. I also had a look in camp
and I didn't see exactly what we need.
In any case I don't see a real reason for explicitly listing all numbers, so I think we should do something like what I wrote above but @dkachuma feel free to also the step size and move it somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I particularly like the idea of min/max component with each fluid. Let me see how I can fit this in.
* @brief Generate a sequence of integers from @p Begin up to (and including) @p End. | ||
*/ | ||
template< integer Begin, integer End > | ||
using IntegerSequence = internal::Increment< camp::make_idx_seq_t< End - Begin + 1 >, Begin >; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! We did have something like this already.
src/coreComponents/constitutive/fluid/multifluid/MultiFluidSelector.hpp
Outdated
Show resolved
Hide resolved
src/coreComponents/constitutive/fluid/multifluid/MultiFluidSelector.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve the common/TypeDispatch
part.
Just a side question, what do you think of the name isThermalType()
to keep the isX()
naming with the thermal()
static method?
* Add component selector * Add test cases * Fix integrated tests * Remove thermal test * Use integer sequence generator * Use constexpr if * thermal -> isThermalType
In IsothermalCompositionalMultiphaseFVMKernels.hpp:2127-2157 and ThermalCompositionalMultiphaseFVMKernels.hpp:1397-1432, we have calls of the form
This expands over all available fluid types and within each expansion, we further expand over all possible numbers of components. The possible number of components is currently set to be from 1 to 5. However, not all fluid types support all these component numbers. For example the
CO2BrineFluid
models support only 2 components. As such we end up here with unnecessary expansions. The end result of all these expansions is to make theCompositionalMultiphaseFVM
class extremely large to compile especially for CUDA builds. This is so bad that some of the fluid models have been commented out in MultiFluidSelector.hpp:39-58 just to be able to compile on GHA.This PR tries to reduce the number of combinations by explicitly listing, for each fluid type, the number of components that it allows. The expansion is a combination of the fluid model type and number of components. Furthermore, not all fluid models support thermal simulations. So the thermal kernels are expanded only over those models that support thermal simulations.
Some other changes
isThermalType()
method.MultiFluidSelector
are honoured.