-
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
Capillary pressure models #1517
Conversation
@@ -120,6 +120,16 @@ inline double to_radians(double degrees) { | |||
return degrees*boost::math::constants::pi<double>()/180.; | |||
} | |||
|
|||
template<typename Type> Type limitValueInInterval(const Type variable, | |||
const Type lower_bound, | |||
const Type upper_bound) |
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.
Better to pass the types by universal reference (Type&&) to avoid copies if the types are expensive to copy.
My bad. Not working this way.
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 type could be integer or real type.
double getCapillaryPressure(const double saturation) const override; | ||
|
||
/// Get capillary pressure. | ||
double getSturation(const double capillary_pressure) 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.
Comment mentions pressure, but this is saturation.
Typo in saturation.
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.
Corrented.
* Note: | ||
* \f[m=1/(1-n)\f] | ||
*/ | ||
class BrookCoreyCapillaryPressure final : public CapillaryPressureSaturation |
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.
Maybe better to call it BrookCoreyCapillaryPressureSaturation to reflect that it is having both p and s and also is derived from CapillaryPressureSaturation.
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 name accordingly.
{ | ||
} | ||
|
||
BrookCoreyCapillaryPressure(const BrookCoreyCapillaryPressure& orig) |
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 can be shortened to
BrookCoreyCapillaryPressure(const BrookCoreyCapillaryPressure&) = default;
Also if copy constructor is present, add the move ctor, and maybe both assignments.
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 the copy constructor because it is not used.
const double _mm; ///< Exponent (<=1.0), n=1/(1-mm). | ||
const double _Pc_max; ///< Maximum capillaray pressure | ||
|
||
const double _perturbation = std::numeric_limits<double>::epsilon(); |
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.
Please add a short comment how the _perturbation is used.
From what I have seen in the code, maybe 'offset' would be a better 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 the name to _minor_offset
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 changes required. Looking good otherwise. I didn't check the formulae.
virtual double getCapillaryPressure(const double saturation) const = 0; | ||
|
||
/// Get capillary pressure. | ||
virtual double getSturation(const double capillary_ressure) const = 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.
same with comment and typo here.
*/ | ||
static std::unique_ptr<CapillaryPressureSaturation> | ||
createBrookCoreyOrVanGenuchten(BaseLib::ConfigTree const& config, | ||
const bool is_brook_corey) |
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'd split it into two different create functions and avoid the boolean parameter.
Adding a third model here would be difficult.
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.
Split it.
c3c7931
to
4a7c08c
Compare
@endJunction Formulas are checked by the unit tests, which use the results from OGS5 as expected data for comparison. |
@@ -89,9 +82,9 @@ class BrookCoreyCapillaryPressure final : public CapillaryPressureSaturation | |||
const double _mm; ///< Exponent (<=1.0), n=1/(1-mm). | |||
const double _Pc_max; ///< Maximum capillaray pressure | |||
|
|||
const double _perturbation = std::numeric_limits<double>::epsilon(); | |||
const double _minor_offset = std::numeric_limits<double>::epsilon(); |
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.
Still needs a description how it is used.
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 a description.
*/ | ||
static std::unique_ptr<CapillaryPressureSaturation> | ||
createVanGenuchten(BaseLib::ConfigTree const& config, | ||
const bool is_brook_corey) |
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.
boolean is no longer needed, isn't 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.
removed.
Jenkins: OGS-6/Linux-PRs-dynamic failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs-dynamic/1579/ |
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 comments from my side.
const double saturation) const | ||
{ | ||
const double S = MathLib::limitValueInInterval( | ||
saturation, _Sr + _minor_offset, _Smax - _minor_offset); |
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.
Are there any mathematical concerns here? p_C(S) is cut off at certain values, but the derivative does not take that into account.
Maybe that's common practice, though...
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 is used to avoid the overflow in std::pow(Se, -n) when S=0. When S=Smax, setting S=Smax - _minor_offset avoids Pc=0. Therefore, it is a numerical 'tricky' to keep computation run with a tiny losing in accuracy.
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.
Thank you for the explanation.
* [4] \f$ P_c^{\mbox{max}}\f$ | ||
*/ | ||
BrookCoreyCapillaryPressureSaturation( | ||
std::array<double, 5> const& parameters) |
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'd prefer to pass the parameters individually rather than as array, because they all mean different things.
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.
{ | ||
/** | ||
\param config ConfigTree object which contains the input data | ||
including <type>BrookCorey</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.
I'd put a check that the type is really BrookCorey.
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.
|
||
/** | ||
\param config ConfigTree object which contains the input data | ||
including <type>vanGenuchten</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.
Likewise: I'd put a check to the function body.
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.
BaseLib::ConfigTree const& config) | ||
{ | ||
//! \ogs_file_param{material_property__porous_medium__porous_medium__capillary_pressure__type} | ||
auto const type = config.getConfigParameter<std::string>("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.
Note: If you'd add checks to the specific create functions, you'd switch to peek...()
here.
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.
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.
@chleh Thanks for above comments. I will improve the classes in the Fluid directories according to these comments once the current work is finished.
* Created on October 28, 2016, 6:05 PM | ||
*/ | ||
|
||
#include "vanGenuchtenCapillaryPressureSaturation.h" |
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.
Almost all files start uppercase. Maybe we should stick to this convention as much as possible.
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.
* [4] \f$ P_c^{\mbox{max}}\f$ | ||
*/ | ||
vanGenuchtenCapillaryPressureSaturation( | ||
std::array<double, 5> const& parameters) |
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.
As before: I'd prefer no array to be used.
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.
for (std::size_t i = 0; i < S.size(); i++) | ||
{ | ||
ASSERT_NEAR(pc[i], pc_model->getCapillaryPressure(S[i]), 1.e-5); | ||
ASSERT_NEAR(S[i], pc_model->getSaturation(pc[i]), 1.e-5); |
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.
That doesn't seem too strict, since S is on the order of 0.1...1
Or is it impossible to decrease this tolerance.
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.
Decreased the tolerance for S to 1e-14.
for (std::size_t i = 0; i < S.size(); i++) | ||
{ | ||
ASSERT_NEAR(pc[i], pc_model->getCapillaryPressure(S[i]), 1.e-5); | ||
ASSERT_NEAR(S[i], pc_model->getSaturation(pc[i]), 1.e-5); |
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.
Maybe it's possible to make the tolerance a bit stricter?
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.
Decreased the tolerance for S to 1e-14.
* as | ||
* \f[p_b=\rho g/\alpha\f] | ||
*/ | ||
class vanGenuchtenCapillaryPressureSaturation final |
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.
Class names should start uppercase.
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.
b7979cf
to
76faead
Compare
added model type check in the function body according to the comments by Christoph
//! \ogs_file_param{material_property__porous_medium__porous_medium__capillary_pressure__type} | ||
config.checkConfigParameter("type", "BrookCorey"); | ||
|
||
//! \ogs_file_param{material_property__porous_medium__porous_medium__capillary_pressure__type__pd} |
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 I'm not mistaken the type
in the string must be BrookCorey
because it is specific for that model.
@chleh Please correct me if I'm talking garbage.
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.
You're right. The line must be
//! \ogs_file_param{material_property__porous_medium__porous_medium__capillary_pressure__BrookCorey__pd}
same for the other doxygen comments.
IMHO since documenting input file parameters is somewhat tricky, it could be fixed when writing documentation. But in order to write documentation it is better to first have test case where this material model is acutally used.
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.
Merge if the config parameter documentation strings are correct. |
OpenGeoSys development has been moved to GitLab. |
This PR presents: