-
Notifications
You must be signed in to change notification settings - Fork 239
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
Relative permeability model #1531
Conversation
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.
Only minor things from my side. In general the code looks good.
For general discussion: We have curves globally in the project data. Should they be exclusively parsed there? Should they be removed from there? Should they be passed to all configuration parsing code?
"The exponent parameter of WettingPhaseVanGenuchten relative\n" | ||
" permeability model, m, must be in an interval of [0, 1]"); | ||
} | ||
//! \ogs_file_param{material_property__porous_medium__porous_medium__relative_permeability__WettingPhaseVanGenuchten__m} |
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.
Typo: __m
at the end?
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.
Corrected to __krel_min
" permeability model, m, must be in an interval of [0, 1]"); | ||
} | ||
//! \ogs_file_param{material_property__porous_medium__porous_medium__relative_permeability__WettingPhaseVanGenuchten__m} | ||
const double krel_max = config.getConfigParameter<double>("krel_min"); |
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.
Typo: krel_max vs. min?
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.
Corrected to krel_min
" permeability model, m, must be in an interval of [0, 1]"); | ||
} | ||
//! \ogs_file_param{material_property__porous_medium__porous_medium__relative_permeability__NonWettingPhaseVanGenuchten__m} | ||
const double krel_max = config.getConfigParameter<double>("krel_min"); |
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.
Typo? max vs min? And __m
above.
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.
Corrected.
"relative permeability model, m, must not be smaller than 1"); | ||
} | ||
//! \ogs_file_param{material_property__porous_medium__porous_medium__relative_permeability__WettingPhaseBrookCoreyOilGas__m} | ||
const double krel_max = config.getConfigParameter<double>("krel_min"); |
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.
Looks like a recurrent pattern :-)
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.
corrected.
/** | ||
\param config ConfigTree object which contains the input data | ||
including <type>NonWettingPhaseBrookCoreyOilGas</type> | ||
and it has a tag of <relative_permeability> |
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.
Do you mean with that that the XML looks like that:
<relative_permeability>
<type>NonWettingPhaseBrookCoreyOilGas</type>
...
<relative_permeability>
?
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.
Yes. The unit test has the syntax.
const double S = MathLib::limitValueInInterval( | ||
saturation_w, _Sr + _minor_offset, _Smax - _minor_offset); | ||
const double Se = (S - _Sr) / (_Smax - _Sr); | ||
const double krel = std::pow(1.0 - Se, 1.0 / 3.0) * |
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.
Minor: there is also std::cbrt()
, which is probably a bit faster.
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.
Changed.
|
||
/// Get relative permeability value. | ||
/// \param saturation_w Non-wetting phase saturation | ||
double getValue(const double saturation_w) const override; |
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.
Likewise: Are derivatives needed?
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.
Added
{ | ||
namespace PorousMedium | ||
{ | ||
class ReletivePermeabilityCurve final : public RelativePermeability |
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.
typo: Relative
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.
Corrected.
const double Se = (S - _Sr) / (_Smax - _Sr); | ||
const double krel = | ||
std::sqrt(Se) * | ||
std::pow(1.0 - std::pow(1.0 - std::pow(Se, 1.0 / _mm), _mm), 2); |
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.
Minor: std::pow
for squaring is rather slow.
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.
Changed.
BaseLib::ConfigTree::onwarning); | ||
auto const& sub_config = conf.getConfigSubtree("relative_permeability"); | ||
//! \ogs_file_attr{prj__material_property__porous_medium__porous_medium__id} | ||
auto const id = sub_config.getConfigAttributeOptional<int>("id"); |
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 you don't need an attribute, you can use sub_config.ignoreAttribute("id")
. I think such a method exists.
Btw: What is the id for? You don't read it in the rest of this PR, so at least you could remove it from the test.
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.
id is reserved for material IDs of elements. We can not input the model parameters as Parameter data because that we may use different models for different material zones in a modelling.
Thanks for the update. |
@chleh For the curve, when I started to implement the curve type material model, I found that the curve should be placed to where it used or belong to. Parsing all curves and saved them as a member of ProjectData is not the best choice. Material models can have curves, BCs can have curves. These curves should be encapsulated to the classes that use them. Besides, input the variables and values in separated vector is not good to perform a manual input. Therefore, I did not add the parsing of the curve for relative permeability to ProjectData class. |
curve_config.getConfigSubtreeList("point")) | ||
{ | ||
const auto& point = | ||
//! \ogs_file_param{material_property__porous_medium__porous_medium__relative_permeability__Curve_curve__points__data} |
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 guess the ending should be '__point__data', one 's' too much.
" <point><data> 0. 0.9 </data></point>" | ||
" <point><data> 0.4 0.5 </data></point>" | ||
" <point><data> 0.9 0.01 </data></point>" | ||
" </curve>" |
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.
Why not use the already existing parser for curves? Why do we need another format?
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.
See my message to Christoph:
For the curve, when I started to implement the curve type material model,
I found that the curve should be placed to where it used or belong to.
Parsing all curves and saved them as a member of ProjectData
is not the best choice. Material models can have curves, BCs can
have curves.
These curves should be encapsulated to the classes that use them.
Besides, input the variables and values in separated vector is not
good to perform a manual input.
Therefore, I did not add the parsing of the curve for relative
permeability to ProjectData class.
If you and others think it is not good to have this XML syntax for a curve. I can use the two array type input by extracting createPiecewiseLinearInterpolation(BaseLib::ConfigTree const& config) from Project Data and saving it to a single file in MathLib/InterpolationAlgorithms.
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.
It would be good to have same syntax for the input of similar (if not equal) data. The project files are not straight forward to understand already, but confusing others with two different options for data input (for curves) is not goining to be an improvement.
I don't mind which syntax is used but it should be consistent.
Personally I prefer the coordinates-values input in long lines, because the splitting of the data into coordinate-value pairs does not carry any additional information.
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 also would prefer one implementation for reading curves.
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.
@TomFischer I prepared one in PR #1529.
@endJunction @TomFischer. If all of you prefer two array to read a curve. I can change it in #1529.
For this PR. I think, I can drop the curve type and move it a new PR after the curve input is unified.
if (i == S.size() - 1) | ||
continue; | ||
// Compare the derivative with numerical one. | ||
ASSERT_NEAR((perm_model->getValue(S[i] + purterbation) - |
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.
typo in perturbation.
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.
Corrected.
BaseLib::ConfigTree::onwarning); | ||
auto const& sub_config = conf.getConfigSubtree("relative_permeability"); | ||
sub_config.ignoreConfigAttribute("id"); | ||
return MaterialLib::PorousMedium::createRelativePermeabilityModel( |
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 you are 'using namespace MaterialLib::PorousMedium' than this can be shorter. Choose one variant.
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.
Removed.
namespace PorousMedium | ||
{ | ||
/** | ||
* \brief BrookCorey oil-gas model: non-wetting phase |
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.
This one seems to be of some van Genuchten type.
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.
Corrected.
return (-std::pow(temp_val, 2. * _mm) / (3. * cbrt1_Se * cbrt1_Se) - | ||
2. * cbrt1_Se * std::pow(temp_val, 2. * _mm - 1.) * | ||
std::pow(Se, (1. - _mm) / _mm)) / | ||
(_Smax - _Sr); |
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.
Unfortunately (and I made that mistake recently too) identifiers starting with an underscore followed by a capital case letter are reserved for the compilers.
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.
Thanks. Just checked and found names of __ and _[Capital letter] are reserved. I remembered that I've ever changed one member name with _[Capital letter] in MathLib for compilation error but did not check the reason.
I will replace the similar names in the capillary pressure classes in a PR following up PR #1529.
|
||
//! \ogs_file_param{material_property__porous_medium__porous_medium__relative_permeability__WettingPhaseVanGenuchten__m} | ||
const double m = config.getConfigParameter<double>("m"); | ||
if (m > 1.0) // m <= 1 |
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 m
is negative is not checked.
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.
Added m<0 in the if-condition
including <type>NonWettingPhaseVanGenuchten</type> | ||
and it has a tag of <relative_permeability> | ||
*/ | ||
std::unique_ptr<RelativePermeability> createNonWettingPhaseVanGenuchten( |
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.
createNonWettingPhaseVanGenuchten
and createWettingPhaseVanGenuchten
seems to be almost identical. Is it possible to refactor common things?
Update Also the following two create functions seems to be almost identical. All functions read the parameters Sr
, Smax
, m
, and krel_min
. The rading and range checking can be unified.
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.
At the beginning, I had only one local function with an option for creating different models. Because of Dima's comment in PR #1517, which has a similar function, I split the single function to four functions. Another consideration is keyword documentation. Although parameter names are the same in the four models, they should be documented for each model individually.
const double _saturation_max; ///< Maximum saturation. | ||
const double _mm; ///< Exponent (>=1.0), n=1/(1-mm). | ||
const double _krel_min; ///< Minimum relative permeability | ||
}; |
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.
The above attributes are use in other classes, too (for instance in NonWettingPhaseBrookCoreyOilGas
). Maybe it makes sense that these classes have a common base class?
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.
Additional, it would be possible to implement limitValueInInterval(saturation)
as a method in this base class. The base class method could use the needed attributes (_saturation_r
, _minor_offset
, and _saturation_max
) and the code would be more readable in my opinion.
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.
@TomFischer limitValueInInterval is also used in capillary pressure classes. Therefore, it was added to MathTool
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.
Moved two common members, _saturation_r and saturation_max to the base class.
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.
When the curves input data question is solved in this or in another but consistent way ⏩
|
||
//! \ogs_file_param{material_property__porous_medium__porous_medium__relative_permeability__NonWettingPhaseBrookCoreyOilGas__m} | ||
const double m = config.getConfigParameter<double>("m"); | ||
if (m < 1.0) // m >= 1 |
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.
In this modell m < 0
is allowed?
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.
No. m<0 is included in m<1.0.
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.
In other models you check if m
is in [0,1]
. Here you do not do this. So is it allowed to give negative values for m
in this model?
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.
m is a double paramerer. If a negative value is given for it, the value is smaller than 1.0 too, and the program gives a fatal error as well. For Brook-Corey models, m>1.
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.
For van Genuchten model m in[0,1].
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.
Now I understand. Thanks!
When rebase is fine, tests are ok: 👍 |
db2cc2f
to
1272e53
Compare
Thanks for your comments. If there is not further comment, I am going to merge it. |
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.
Mostly comments and some style suggestions.
⏩ when you think there is already enough improvement.
|
||
//! \ogs_file_param{material__porous_medium__relative_permeability__WettingPhaseVanGenuchten__m} | ||
const double m = config.getConfigParameter<double>("m"); | ||
if (m < 0. || m > 1.0) // m <= 1 |
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.
the comment does not match the expression. I'd remove the comments; the reason is already given in the error message.
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.
removed it here and one another in other line.
NonWettingPhaseVanGenuchten(const double Snr, const double Snmax, | ||
const double m, const double krel_min) | ||
: RelativePermeability(1. - Snmax, 1. - Snr), | ||
_mm(m), |
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.
just curious, why not _m
? It would be consistent with the parameter name.
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.
Changed to _m.
double getdValue(const double saturation_w) const override; | ||
|
||
private: | ||
const double _mm; ///< Exponent (<=1.0), n=1/(1-mm). |
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.
In the corresponding create function you check if this value is in [0,1] interval; the comment here is different. I can not say which one is correct, but if the checks and comments are kept together it is easier to spot mistakes.
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.
Changed the comment to
///< Exponent m, m in [0, 1], n=1/(1-m).
virtual double getdValue(const double saturation) const = 0; | ||
|
||
protected: | ||
/** A small number for an offset to set the bound of S, the saturation, such |
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.
Sometimes doxygen comments are in /** */ style, sometimes in ///. Choose one, stick to it.
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 use /// for comments be less than two lines, and /** */ for those more than two lines.
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.
:) the rule is fine, but this particular one has only two lines ;)
But keep it like it is, I don't mind it too much.
{ | ||
const char xml[] = | ||
// Should be <relative_permeability id="0"> | ||
"<relative_permeability id=\"0\">" |
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.
The comment's statement is true. Probably the comment is no longer needed.
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.
Removed.
… and corrected the documenations for some keywords.
with respect to saturation.
according to the comments by Dima.
OpenGeoSys development has been moved to GitLab. |
This PR presents: