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.

Sign up for GitHub

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

Jump to bottom

Implement Spycher-Pruess solubility tables #2820

Merged
merged 42 commits into from
Mar 15, 2024
Merged

Implement Spycher-Pruess solubility tables #2820

merged 42 commits into from
Mar 15, 2024

Conversation

dkachuma
Copy link
Contributor

@dkachuma dkachuma commented Nov 15, 2023 •
edited
Loading

Populates CO2 solubility and water vapourisation tables using the formulation from Spycher et al. (2003)

  • Switch from Duan & Sun (2003) model to Spycher et al. (2003) model done by adding an extra parameter to the flash file.
  • Values are precalculated for a range of pressure and temperature and stored as a 2D table which is used in the "flash" (CO2Solubility).
  • Currently salinity is passed but not yet used. This will have to be extended using Spycher & Pruess (2005)
     to include salinity.

@dkachuma dkachuma added the type: feature New feature or request label Nov 15, 2023
@dkachuma dkachuma self-assigned this Nov 15, 2023
@dkachuma dkachuma added the spe11 spe11-required-feature label Nov 15, 2023
@dkachuma dkachuma mentioned this pull request Nov 17, 2023
Open
24 tasks
jafranc
jafranc approved these changes Feb 9, 2024
View reviewed changes
Copy link
Contributor

@jafranc jafranc left a comment

Choose a reason for hiding this comment

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

Great addition to GEOS capability.
Thank you @dkachuma ! Just few comments on class hierarchy

src/coreComponents/constitutive/fluid/multifluid/CO2Brine/functions/CO2Solubility.cpp Outdated
string const solubilityModel = 10 < inputParams.size() ? inputParams[10] : "DuanSun";
FunctionManager & functionManager = FunctionManager::getInstance();

if( solubilityModel == "SpycherPruess" )
Copy link
Contributor

Choose a reason for hiding this comment

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

would go for enum compare rather than string, but as long as it works :)
see ENUM_STRINGS IIRC

src/coreComponents/constitutive/fluid/multifluid/CO2Brine/functions/CO2Solubility.cpp Outdated
}
else if( solubilityModel == "DuanSun" )
{
m_CO2SolubilityTable = makeSolubilityTable( inputParams, m_modelName, functionManager );
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to have CO2Solubility to be pure virtual and each model in a class, hence CO2SolubilitySpycherPruess and CO2SolubilityDuanSun

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 considered adding another solubility class. However, CO2Solubility is used as a template parameter in CO2BrineFluid. Adding a different class likely means adding a new instantiation of the template and probably a new catalog entry. This is probably OK but I didn't want to add to the proliferation of catalog types especially considering that the change in functionality is quite minimal.

In any case I think I should refactor CO2Solubility and clearly separate the Duan&Sun part from the Spycher part.

src/coreComponents/constitutive/fluid/multifluid/CO2Brine/functions/CO2Solubility.cpp Outdated
else
{
GEOS_THROW( GEOS_FMT( "The CO2Solubility model {} is not known."
" This should be either 'DuanSun' or 'SpycherPruess'.", solubilityModel ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would go as @MelReyCG did to list enum values if possible

src/coreComponents/constitutive/fluid/multifluid/CO2Brine/functions/PVTFunctionHelpers.hpp Show resolved Hide resolved
src/coreComponents/unitTests/constitutiveTests/CMakeLists.txt Show resolved Hide resolved
...reComponents/constitutive/fluid/multifluid/CO2Brine/functions/CO2SolubilitySpycherPruess.cpp Outdated
}
}

// Truncate negative solubility and warn
Copy link
Contributor

Choose a reason for hiding this comment

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

This, for instance, could go in abstract base class

@dkachuma dkachuma requested a review from jafranc March 4, 2024 22:44
jafranc
jafranc approved these changes Mar 6, 2024
View reviewed changes
Copy link
Contributor

@jafranc jafranc left a comment

Choose a reason for hiding this comment

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

Great refactoring/extension !!
Thank you @dkachuma

src/coreComponents/constitutive/fluid/multifluid/CO2Brine/functions/CO2Solubility.cpp Show resolved Hide resolved
src/coreComponents/constitutive/fluid/multifluid/CO2Brine/functions/CO2Solubility.cpp Show resolved Hide resolved
src/coreComponents/constitutive/fluid/multifluid/CO2Brine/functions/CO2Solubility.cpp Outdated Show resolved Hide resolved
@dkachuma dkachuma added the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Mar 7, 2024
@paveltomin paveltomin added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Mar 12, 2024
@TotoGaz
Copy link
Contributor

TotoGaz commented Mar 13, 2024

Does this require rebaselines?

@dkachuma
Copy link
Contributor Author

Does this require rebaselines?

No it shouldn't.

@TotoGaz TotoGaz added flag: no rebaseline Does not require rebaseline and removed ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Mar 14, 2024
@paveltomin paveltomin added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Mar 15, 2024
@CusiniM CusiniM removed the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Mar 15, 2024
@CusiniM CusiniM merged commit 8300a43 into develop Mar 15, 2024
25 checks passed
@CusiniM CusiniM deleted the feature/dkachuma/spycher-pruess-solubility-tables branch March 15, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jafranc jafranc jafranc approved these changes

Assignees

@dkachuma dkachuma

Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs flag: no rebaseline Does not require rebaseline spe11 spe11-required-feature type: feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dkachuma @TotoGaz @jafranc @paveltomin @CusiniM