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

Implementation of HC process #1758

Merged
merged 15 commits into from Apr 27, 2017

Conversation

Projects
None yet
3 participants
@TomFischer
Member

TomFischer commented Apr 5, 2017

Missing things:

  • Documentation of parameters
  • Documentation of benchmarks Marc is working on this, however code review can start.
  • goswamy benchmark

For discussion:

  • process name: HC or ComponentTransport or ...
  • search length for boundary conditions

Update Changing the default search length breaks the benchmark 'ogs-TES_zeolite_discharge_small'. So I removed the commit.

@TomFischer TomFischer force-pushed the TomFischer:HC-Implementation branch 10 times, most recently from 812f833 to e4595c9 Apr 5, 2017

@TomFischer TomFischer force-pushed the TomFischer:HC-Implementation branch 2 times, most recently from b30e0a0 to a7bb07e Apr 18, 2017

@TomFischer TomFischer force-pushed the TomFischer:HC-Implementation branch 3 times, most recently from 338866e to b84b66d Apr 19, 2017

p_nodal_values);
double const velocity_magnitude = velocity.norm();
GlobalDimMatrixType const& I(

This comment has been minimized.

@wenqing

wenqing Apr 21, 2017

Member

The definition, GlobalDimMatrixType const& I, can be moved to before the start of this loop.

auto const retardation_factor =
_process_data.retardation_factor(t, pos)[0];
auto KCC = local_K.template block<num_nodes, num_nodes>(0, 0);

This comment has been minimized.

@wenqing

wenqing Apr 21, 2017

Member

It is possible to move lines 183 to 189 about the matrix blocks before the start of the integration loop?

PorousMediaProperties porous_media_properties{
createPorousMediaProperties(mesh, porous_medium_configs)};
//! \ogs_file_param{prj__processes__process__ComponentTransport__fluid}

This comment has been minimized.

@wenqing

wenqing Apr 21, 2017

Member

Suggest to use class FluidProperties, which contains density, viscosity, specific heat capacity and thermal conductivity (optional). There is a creator for FluidProperties, please see
https://github.com/ufz/ogs/blob/master/ProcessLib/LiquidFlow/CreateLiquidFlowMaterialProperties.cpp#L49

{
namespace ComponentTransport
{
PorousMediaProperties createPorousMediaProperties(

This comment has been minimized.

@wenqing

wenqing Apr 21, 2017

Member

👍 . This class can be general one for all fluid flow processes. After this PR merged, I may refractory the other flow processes by using this class to reduce the source code duplication about porous medium among process classes.

void operator=(ComponentTransportProcessData&&) = delete;
PorousMediaProperties porous_media_properties;
std::unique_ptr<MaterialLib::Fluid::FluidProperty> viscosity_model;

This comment has been minimized.

@wenqing

wenqing Apr 21, 2017

Member

As the comment given about, you can replace

  std::unique_ptr<MaterialLib::Fluid::FluidProperty> viscosity_model;
  std::unique_ptr<MaterialLib::Fluid::FluidProperty> fluid_density;

with

  const std::unique_ptr<MaterialLib::Fluid::FluidProperties>
    _fluid_properties;

Here is an example to use FluidProperties:
https://github.com/ufz/ogs/blob/master/ProcessLib/LiquidFlow/LiquidFlowMaterialProperties.cpp#L75

@TomFischer TomFischer force-pushed the TomFischer:HC-Implementation branch from b84b66d to 6cd41b0 Apr 24, 2017

@TomFischer

This comment has been minimized.

Member

TomFischer commented Apr 24, 2017

I compared the run times on my laptop with and without the last commit (c0bfb08). The previous version (without FluidProperties) takes circa 79 seconds while the new version takes ~ 81.6 seconds.

@wenqing

This comment has been minimized.

Member

wenqing commented Apr 24, 2017

@TomFischer If the fluid properties do not depend on density, the extra operation to access members of individual fluid property class is to access an entry of the property array

const std::array<std::unique_ptr<FluidProperty>, FluidPropertyTypeNumber>
        _property_models;

class FluidProperties enables the temperature density dependent fluid property models, e.g. WaterViscosityIAPWS.

@wenqing

👍

@TomFischer TomFischer force-pushed the TomFischer:HC-Implementation branch from c0bfb08 to d249206 Apr 25, 2017

@@ -22,7 +22,8 @@ enum class PropertyVariableType
T = 0, ///< temperature.
p = 1, ///< pressure.
rho = p, ///< density. For some models, rho substitutes p
number_of_variables = 2 ///< Number of property variables.
C = 2,

This comment has been minimized.

@endJunction

endJunction Apr 25, 2017

Member

short comment

MaterialLib::Fluid::PropertyVariableType::C)] = C_int_pt;
vars[static_cast<int>(
MaterialLib::Fluid::PropertyVariableType::p)] = p_int_pt;
auto const density_water = _process_data.fluid_properties->getValue(

This comment has been minimized.

@endJunction

endJunction Apr 25, 2017

Member

better to call it density_fluid (or simply density, if there is no other), it's not always water...

_process_data.fluid_properties->getValue(
MaterialLib::Fluid::FluidPropertyType::Viscosity, vars);
GlobalDimMatrixType const perm_over_visc =

This comment has been minimized.

@endJunction

endJunction Apr 25, 2017

Member

Better to call it K_over_mu as it is done below.

std::move(porous_media_properties),
// std::move(viscosity_model),
fluid_reference_density,
// std::move(fluid_density),

This comment has been minimized.

@endJunction

endJunction Apr 25, 2017

Member

drop the comments or explain why needed

permeability_conf));
// Parameter for the specific storage.
auto const& storage_conf =

This comment has been minimized.

@endJunction

endJunction Apr 25, 2017

Member

typo: config ;)
and the comment above is misleading...

double t, SpatialPosition const& pos) const;
Eigen::MatrixXd const& getIntrinsicPermeability(double t,
SpatialPosition const& pos) const;

This comment has been minimized.

@endJunction

endJunction Apr 25, 2017

Member

maybe formatting... git clang-format ufz/master is good!

@endJunction

Looks promising.

In the assembly part I'd maybe try to extract special functions for velocity computation etc....

@TomFischer TomFischer force-pushed the TomFischer:HC-Implementation branch 2 times, most recently from f161f44 to d01724f Apr 26, 2017

@TomFischer TomFischer force-pushed the TomFischer:HC-Implementation branch from d01724f to 9c8bc69 Apr 27, 2017

@TomFischer TomFischer force-pushed the TomFischer:HC-Implementation branch from 9c8bc69 to 4589c3c Apr 27, 2017

@TomFischer TomFischer merged commit 4a99140 into ufz:master Apr 27, 2017

2 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@TomFischer TomFischer deleted the TomFischer:HC-Implementation branch Apr 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment