-
Notifications
You must be signed in to change notification settings - Fork 91
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
Add hybrid FVM compositional multiphase solver #901
Add hybrid FVM compositional multiphase solver #901
Conversation
7ce4890
to
97f3858
Compare
97f3858
to
d09c568
Compare
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.
Great work!
I hope the compile times are ok ;)
FaceManager * const faceManager = meshLevel.getFaceManager(); | ||
{ | ||
faceManager->registerWrapper< array1d< real64 > >( viewKeyStruct::facePressureString )-> | ||
setPlotLevel( PlotLevel::LEVEL_0 )-> | ||
setRegisteringObjects( this->getName() )-> | ||
setDescription( "An array that holds the pressures at the faces." ); | ||
} |
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.
Seeing as this is done in the base class, do we want to always register face pressure, even for classical FVM? It could be useful for face-based BC, but it's not clear if anyone would want to implement them in the compositional solver.
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 it would be useful to have face-based boundary condition for the classical FVM compositional solver as well. For instance, it would be convenient to have this face-based BC for a rigorous refinement study or a scalability test. That being said, I don't really realize how much work it would take to implement them for the compositional case. I can have look into that.
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 to have face b.c. for the compositional solver and use them for refinement studies we would have to specify all other fields as well (i.e., faceCompositions and eventually faceTemperature). I do agree that it could be useful though.
@@ -651,19 +601,21 @@ void CompositionalMultiphaseFlow::BackupFields( MeshLevel & mesh ) const | |||
{ | |||
phaseDensOld[ei][ip] = phaseDens[ei][0][ip]; | |||
phaseVolFracOld[ei][ip] = phaseVolFrac[ei][ip]; | |||
phaseMobOld[ei][ip] = phaseMob[ei][ip]; |
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.
Where is previous time step mobility used, if fluxes are always evaluated implicitly?
(I'm reading top to bottom, so hopefully I will find the answer below and won't forget to delete this comment.)
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 did find the answer btw, it was in the residual normalizer
...mponents/physicsSolvers/fluidFlow/CompositionalMultiphaseHybridFVMAssemblerHelperKernels.hpp
Outdated
Show resolved
Hide resolved
src/coreComponents/physicsSolvers/fluidFlow/CompositionalMultiphaseHybridFVM.cpp
Outdated
Show resolved
Hide resolved
src/coreComponents/physicsSolvers/fluidFlow/CompositionalMultiphaseHybridFVM.cpp
Outdated
Show resolved
Hide resolved
@klevzoff here are some compilation timings to evaluate the impact of this PR: I am doing:
and reporting the Quartz with Lassen with and I do see on Travis that the One temporary thing that may increase the compilation time of the hybrid solver(s) is the switchyard over the number of faces before the kernel launch (but I am not sure): |
Yes, this is exactly why I asked about compile times. Thanks for checking!
I'm not yet sure how well that's going to work. As we know, in general polyhedral grid number of faces can get quite large, and the kernel stack space requirements grow quadratically (NF x NF local arrays), so even with a limit of 12 or 16 faces it has the potential to severely limit the occupancy (number of thread blocks executing simultaneously) or even make the kernel impossible to launch on device (I've faced a similar issue with some of the I would be in favor of keeping the switchyard so we can keep optimal performance for the fixed element type grids (hex/tets/wedges). We could have the switchyard dispatch only the general polyhedral subregions to the "runtime" version of the kernel (one that takes
One simple thing to try is forcing explicit template instantiations in |
@klevzoff I just pushed a new version of the code with explicit template instantiation to see if I can now pass the Quartz with Lassen with To make this work, I moved all the kernel functions to One thing that I did not manage to do is to make the explicit template instantiation work with the "fourth" template on the type of inner product in |
@klevzoff I managed to reintroduce the four templates that I initially had in
With these two changes, the compilation times on Lassen are now fine (comparable with |
Does it significantly hurt our capability to experiment with different variations of hybrid FVM? Seems like you've put a good amount of work into it and it would be a shame to have that code sit unused. This is probably something to do in separate PR (this one should go in soon!), but we can still try multiple translation units idea, unless you already have. Basically, it would go this way (all names are arbitrary):
Again, this is just an idea to try in the future. This PR is good as it is! |
@klevzoff Thanks for the plan outlined above, I am going to start working on it as soon as this PR is merged (hopefully today). I think limiting the number of inner product types as temporary workaround is fine for now, as long as we have the TPFA inner product and a consistent inner product, which is the case in the current version of this PR. |
This PR implements a mixed hybrid solver for compositional multiphase flow with buoyancy. I organized the compositional multiphase flow solver as follows:
CompositionalMultiphaseBase
that contains the property update functions and the computation of the accumulation termCompositionalMultiphaseFVM
that implements the cell-centered TPFA-MPFA flux assembly and boundary conditions.CompositionalMultiphaseHybridFVM
that implements the mixed hybrid flux assembly.Note that the large number of lines changed is due to an integrated test using an imported mesh file (i.e, not actual code changes).
To-do list:
https://github.com/GEOSX/integratedTests/pull/119