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

New MPL property: Liakopoulos relative permeability and saturation #2657

Merged
merged 18 commits into from Sep 23, 2019

Conversation

@Scinopode
Copy link
Contributor

Scinopode commented Sep 11, 2019

Relative permeability and saturation properties for the generalized material property library as used in the Liakopoulos multiphase benchmark.

  1. Feature description was added to the changelog
  2. Tests covering your feature were added?
  3. Any new feature or behavior change was documented?
@Scinopode Scinopode force-pushed the Scinopode:mpl_liakopoulos_properties branch 2 times, most recently from 0efea5c to 7fac9a8 Sep 12, 2019
@Scinopode Scinopode force-pushed the Scinopode:mpl_liakopoulos_properties branch 3 times, most recently from fcd7d5e to 8abeba3 Sep 17, 2019
@Scinopode Scinopode force-pushed the Scinopode:mpl_liakopoulos_properties branch from 8abeba3 to 90f1747 Sep 18, 2019
@endJunction endJunction requested a review from wenqing Sep 18, 2019
auto const s_L = _medium->property(PropertyType::saturation)
.template value<double>(variable_array, pos, t);
auto const s_L_res = _residual_liquid_saturation;
if (s_L < s_L_res)

This comment has been minimized.

Copy link
@wenqing

wenqing Sep 18, 2019

Member

Suggestion to restrict the function within [s_r, s_max]. For example,

Suggested change
if (s_L < s_L_res)
const double s_within_range = std::min(std::max(s_L_res, s_L), 1.0); //or replace 1.0 with s_L_max

Then replace s_L with s_within_range in the following lines.

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 19, 2019

Author Contributor

OK, i do so. However, this affects the dry condition only, which is not considered in the Liakopoulos experiment (Saturation does not drop below .7 or so). Anyway, this is a better approach.

/**
* \class RelPermLiakopoulos
* \brief Relative permeability function for Liakopoulos Benchmark
* \details This property must be a medium property, it

This comment has been minimized.

Copy link
@wenqing

wenqing Sep 18, 2019

Member

It would be better to add its formula in this documentation.

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 19, 2019

Author Contributor

Agreed. I added the equations.

const double p_cap = std::get<double>(
variable_array[static_cast<int>(Variable::capillary_pressure)]);

if ((p_cap < 0.) || (p_cap >= _p_cap_max))

This comment has been minimized.

Copy link
@wenqing

wenqing Sep 18, 2019

Member

Introducing a restricted p_cap as

Suggested change
if ((p_cap < 0.) || (p_cap >= _p_cap_max))
const double restricted_p_cap =std::min(std::max(p_cap, 0.), _p_cap_max));
return -_parameter_a * _parameter_b * std::pow(restricted_p_cap, _parameter_b - 1.);

This comment has been minimized.

Copy link
@endJunction

endJunction Sep 18, 2019

Member

I'd say that returning a zero is faster than doing additional computations. Need's a measurement, of course.
Also in my opinion the if condition is easier to understand...

This comment has been minimized.

Copy link
@wenqing

wenqing Sep 18, 2019

Member

If p_cap<0, return 0.
If if p_cap >=_p_cap_max, return dvalue at p_cap_max.

This comment has been minimized.

Copy link
@endJunction

endJunction Sep 18, 2019

Member

I see. I didn't understand it from your original comment.

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 19, 2019

Author Contributor

OK, did that.

const double p_cap = std::get<double>(
variable_array[static_cast<int>(Variable::capillary_pressure)]);

if ((p_cap < 0.) || (p_cap >= _p_cap_max))

This comment has been minimized.

Copy link
@wenqing

wenqing Sep 18, 2019

Member

Here the same.

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 19, 2019

Author Contributor

OK

@Scinopode Scinopode force-pushed the Scinopode:mpl_liakopoulos_properties branch from 90f1747 to 215e558 Sep 19, 2019
Copy link
Member

wenqing left a comment

Looks good except one question from my side.

auto const s_L_res = _residual_liquid_saturation;
auto const k_rel_min_GR = _min_relative_permeability_gas;

if (s_L <= s_L_res)

This comment has been minimized.

Copy link
@wenqing

wenqing Sep 19, 2019

Member

Here, you do not want to restrict s_L. Don't you?

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 23, 2019

Author Contributor

I'm not sure what you mean. If I don't restrict it, I obtain a negative effective saturation for s_L < s_L_res and can't solve for k_rel_GR... Did I miss something?

* computes the permeability reduction due to saturation as function
* of capillary pressure.
*
* Wetting (liquid) phase erlative permeability is given by the empirical

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 23, 2019

Member
Suggested change
* Wetting (liquid) phase erlative permeability is given by the empirical
* Wetting (liquid) phase relative permeability is given by the empirical

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 23, 2019

Author Contributor

ok

private:
Medium* _medium = nullptr;
/**
Parameters for Liakopoulos relative permeability:

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 23, 2019

Member

clang format?

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 23, 2019

Author Contributor

ok

* \brief
*
* \copyright
* Copyright (c) 2012-2018, OpenGeoSys Community (http://www.opengeosys.org)

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 23, 2019

Member
Suggested change
* Copyright (c) 2012-2018, OpenGeoSys Community (http://www.opengeosys.org)
* Copyright (c) 2012-2019, OpenGeoSys Community (http://www.opengeosys.org)

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 23, 2019

Author Contributor

ok

* \brief
*
* \copyright
* Copyright (c) 2012-2018, OpenGeoSys Community (http://www.opengeosys.org)

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 23, 2019

Member
Suggested change
* Copyright (c) 2012-2018, OpenGeoSys Community (http://www.opengeosys.org)
* Copyright (c) 2012-2019, OpenGeoSys Community (http://www.opengeosys.org)

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 23, 2019

Author Contributor

ok

* \brief
*
* \copyright
* Copyright (c) 2012-2018, OpenGeoSys Community (http://www.opengeosys.org)

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 23, 2019

Member
Suggested change
* Copyright (c) 2012-2018, OpenGeoSys Community (http://www.opengeosys.org)
* Copyright (c) 2012-2019, OpenGeoSys Community (http://www.opengeosys.org)

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 23, 2019

Author Contributor

ok

private:
Medium* _medium = nullptr;
/**
Parameters for Liakopoulos saturation curve taken from:

This comment has been minimized.

Copy link
@TomFischer

TomFischer Sep 23, 2019

Member

Clang format.

This comment has been minimized.

Copy link
@Scinopode

Scinopode Sep 23, 2019

Author Contributor

ok

Copy link
Member

TomFischer left a comment

Small things to correct. Then

@Scinopode Scinopode force-pushed the Scinopode:mpl_liakopoulos_properties branch from 215e558 to 86b7ebd Sep 23, 2019
@endJunction endJunction merged commit f66c2a8 into ufz:master Sep 23, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/jenkins/pr-merge This commit cannot be built
Details
ufz.ogs #20190923.3 failed
Details
deploy/netlify Deploy preview ready!
Details
@Scinopode Scinopode deleted the Scinopode:mpl_liakopoulos_properties branch Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.