Skip to content
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

Create compositional multiphase fluid #2769

Merged
merged 29 commits into from
Nov 9, 2023

Conversation

dkachuma
Copy link
Contributor

@dkachuma dkachuma commented Oct 18, 2023

  • Creates a native implementation of CompositionalMultiphaseFluid
  • There is currently no implementation behind this to keep the PR smaller. The required values are currently hard coded constants. This PR focusses on the definition of the catalog entries that will face the user. The current purpose it to ensure that the mechanism is in place to be able to run this through the PVTDriver and check that the hard-coded values are output. Follow up PRs will link the actual static calculation kernels.
  • Renames the implementation that depends on PVTPackage to CompositionalMultiphaseFluidPVTPackage
  • Introduces 2 fluid types CompositonalTwoPhaseFluidSoaveRedlichKwong and CompositonalTwoPhaseFluidPengRobinson
  • Removes the equationsOfState input. This is implied from the element tag.
  • Adds componentCriticalVolume which is optional. When not supplied this is calculated using Ihmels' (2010) correlation.

@dkachuma dkachuma mentioned this pull request Oct 19, 2023
24 tasks
@dkachuma dkachuma self-assigned this Oct 26, 2023
@dkachuma dkachuma marked this pull request as ready for review October 31, 2023 02:29
}

// 2. Compute phase fractions and phase component fractions
m_flash.compute( pressure,
Copy link
Contributor

Choose a reason for hiding this comment

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

it overall looks ok, following current GEOS practices for fluids
but i still do not fully understand why flash compute and phase computes can't become separate kernels?
would simplify the code imho and avoid zillions of template functions
@rrsettgast @klevzoff @francoishamon need you opinion please

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe i could try to implement what i am talking about for a simple fluid to see how it would look...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, a dummy design would be helpful. For the moment I suggest we keep this PR moving forward so we can get baselines in the code, but we can always revisit the design.

Copy link
Contributor

@klevzoff klevzoff Oct 31, 2023

Choose a reason for hiding this comment

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

The original design was influenced primarily by PVTPackage which provided a single entry point for all calculations. This does not need to be the case any more.

However a secondary reason concerns performance, especially on GPUs. Split kernel design introduces additional latency and requires input data to be brought from DRAM into SM registers multiple times. Fused kernels are typically much more efficient in that sense and also allow intermediate computation results can be reused in-kernel without storing them into memory. This may be less of a concern if 90% of simulation time is taken by the linear solver.

In addition, our current approach to running these kernels is to have the solver do it rather than the model. This allows fluid compute to be fused into another kernel with a single level of model dispatch, as is the case with Dirichlet boundary conditions in CompositionalMultiphaseFVM, which avoids the need to allocate memory to store all fluid properties at those BC locations. I don't see an easy way to do this with split kernel approach, but I'm open to experiments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will presently carry on as is and welcome ideas on how to rework this.

{
string const expectedWaterPhaseNames[] = { "water" };
return PVTProps::PVTFunctionHelpers::findName( m_phaseNames, expectedWaterPhaseNames, viewKeyStruct::phaseNamesString() );
return -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a plan yet for how we handle the water phase index in the new design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently I'm not sure how the phaseNames input fits into all this. I haven't put this in since my current implementations are 2-phase with liquid and vapour. Once I start doing implementations with water, this will have to be worked out.

@dkachuma dkachuma closed this Nov 3, 2023
@dkachuma dkachuma reopened this Nov 3, 2023
@dkachuma
Copy link
Contributor Author

dkachuma commented Nov 3, 2023

Outstanding issues:

  • Hard-coded constant values: Subsequent PRs will link the values to the actual static calculation functions.
  • Removal of possibly redundant/duplicate interface: Has to be done across fluid models in a follow up task.

@joshua-white joshua-white added ci: run CUDA builds Allows to triggers (costly) CUDA jobs and removed flag: ready for review labels Nov 3, 2023
@joshua-white
Copy link
Contributor

I went ahead and added the "ready to be merged" flag. I assume no rebaselines are required.

@dkachuma
Copy link
Contributor Author

dkachuma commented Nov 3, 2023

I went ahead and added the "ready to be merged" flag. I assume no rebaselines are required.

No. Shouldn't require rebaselines.

@paveltomin
Copy link
Contributor

I went ahead and added the "ready to be merged" flag. I assume no rebaselines are required.

No. Shouldn't require rebaselines.

most probably it does require rebaseline because you changed registerWrapper in some places and registered something in catalog?

@dkachuma
Copy link
Contributor Author

dkachuma commented Nov 4, 2023

most probably it does require rebaseline because you changed registerWrapper in some places and registered something in catalog?

But these are new types which shouldn't exist yet in any of the tests.

@joshua-white
Copy link
Contributor

most probably it does require rebaseline because you changed registerWrapper in some places and registered something in catalog?

But these are new types which shouldn't exist yet in any of the tests.

The situation will be obvious when we try to run the integrated tests :-) @dkachuma, can you (have you) run on Quartz and Lassen? We've had a few instances recently of PRs getting in without confirming tests.

@dkachuma
Copy link
Contributor Author

dkachuma commented Nov 7, 2023

The situation will be obvious when we try to run the integrated tests :-) @dkachuma, can you (have you) run on Quartz and Lassen? We've had a few instances recently of PRs getting in without confirming tests.

I have run the tests locally on pecan. But as of yet I don't have access to Quartz or Lassen.

@castelletto1
Copy link
Contributor

The situation will be obvious when we try to run the integrated tests :-) @dkachuma, can you (have you) run on Quartz and Lassen? We've had a few instances recently of PRs getting in without confirming tests.

I have run the tests locally on pecan. But as of yet I don't have access to Quartz or Lassen.

Tests pass on Quartz. New baselines are not needed.

@castelletto1 castelletto1 merged commit 547798f into develop Nov 9, 2023
@castelletto1 castelletto1 deleted the feature/dkachuma/compositional-fluid branch November 9, 2023 15:46
ouassimkh pushed a commit that referenced this pull request Feb 16, 2024
* Create copy of CompositionalMultiphaseFluid
* Create another copy of CompositionalMultiphaseFluid
* Rename files
* Add a parallel CompositionalMultiphaseFluid
* Rename catalog objects
* Add some reasonable non-zero values for properties
* Create template parameterised Compositional multiphase fluid
* Create phase models
* Replace template parameter pack with individual parameters
* Naming conventions
* Rename compositional density
* Fix unit test
* Remove catalog functions
* Remove impl file
* Rename CompositionalMultiphaseFluidPVT to CompositionalMultiphaseFluidPVTPackage
* Remove superfluous comment
* Pass density to viscosity kernel
* Lump component molecular weight and component names into ComponentProperties
* Implement water phase index
* Remove third phase from kernel wrapper
* Remove unused variable
* Remove component properties from individual models
* Remove unused variables
* Updating schema
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants