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

[PL] Multi-component transport process #2304

Merged
merged 12 commits into from Jan 23, 2019

Conversation

Projects
None yet
3 participants
@renchao-lu
Copy link
Member

renchao-lu commented Dec 21, 2018

This pull request aims at extending the functionality of component transport process which now can be applicable for multiple component migration. Note that, this work is done using the newly developed multi-phase/-component library (#2303).

@renchao-lu renchao-lu force-pushed the renchao-lu:MultiComponentTransportProcess branch 3 times, most recently from 5d41928 to 8300206 Dec 21, 2018

@renchao-lu renchao-lu force-pushed the renchao-lu:MultiComponentTransportProcess branch 3 times, most recently from e39af45 to a8bcf7b Jan 11, 2019

@bilke bilke force-pushed the ufz:master branch from 2a347a0 to a016b45 Jan 17, 2019

@endJunction endJunction force-pushed the renchao-lu:MultiComponentTransportProcess branch from a8bcf7b to 36df923 Jan 17, 2019

@renchao-lu renchao-lu force-pushed the renchao-lu:MultiComponentTransportProcess branch 3 times, most recently from e12cc6c to 440b085 Jan 17, 2019

static const int pressure_index = 0;
static const int pressure_size = ShapeFunction::NPOINTS;
// per component
static const int concentration_size = ShapeFunction::NPOINTS;

This comment has been minimized.

@endJunction

endJunction Jan 18, 2019

Member

Please decide how to call a thing: component or concentration. Stick to it.
Maybe single_component_size.
It might be, that I don't yet grasp the difference btw the component and concentration.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

rewritten the comment.

@@ -131,48 +152,119 @@ class LocalAssemblerData : public ComponentTransportLocalAssemblerInterface
auto local_b = MathLib::createZeroedVector<LocalVectorType>(
local_b_data, local_matrix_size);

// Nodal DOFs include pressure
auto const num_comp = num_nodal_dof - 1;

This comment has been minimized.

@endJunction

endJunction Jan 18, 2019

Member
Suggested change
auto const num_comp = num_nodal_dof - 1;
auto const number_of_components = num_nodal_dof - 1;

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

Fixed.

auto local_p = Eigen::Map<const NodalVectorType>(
&local_x[pressure_index], pressure_size);

for (int comp_id = 0; comp_id < num_comp; ++comp_id)

This comment has been minimized.

@endJunction

endJunction Jan 18, 2019

Member
Suggested change
for (int comp_id = 0; comp_id < num_comp; ++comp_id)
for (int component_id = 0; component_id < number_of_components; ++component_id)

Also I suggest to keep variable's usage and definition close: move definition of the number_of_components before the for-loop.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

Fixed and stuck the condition variable 'number_of_component' with the follow-up for-loop.


// Use the fluid density model to compute the density
// TODO: concentration of which component as the argument for

This comment has been minimized.

@endJunction

endJunction Jan 18, 2019

Member
Suggested change
// TODO: concentration of which component as the argument for
// TODO (renchao): concentration of which component as the argument for

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

Added.

auto const concentration_index = 1 * ShapeFunction::NPOINTS;
// assuming that fluid density always depends on concentration of the
// first component.
// get local_C and local_p

This comment has been minimized.

@endJunction

endJunction Jan 18, 2019

Member

Drop this line. The comment is useless above (where it is correct). Here it is wrong. Don't copy-paste comments.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

Removed.


// get all process variables stored in a vector before allocation
// pressure first, concentration then
auto const unalloc_process_variables = findProcessVariables(

This comment has been minimized.

@endJunction

endJunction Jan 18, 2019

Member

What do you mean by unalloc?

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

Given a more accurate name for this variable.

namespace ProcessLib
{
namespace ComponentTransport
{
std::unique_ptr<Process> createComponentTransportProcess(
MeshLib::Mesh& mesh,

This comment has been minimized.

@endJunction

endJunction Jan 18, 2019

Member

clang-format

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

reformatted

auto var_names = pv_config.getConfigParameterList<std::string>(tag);

if (var_names.empty())
OGS_FATAL("Config tag <%s> is not found.", tag.c_str());

This comment has been minimized.

@endJunction

endJunction Jan 18, 2019

Member

Message is wrong. The tag was found, but the result is empty.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

Rewritten the error message.

var_name.c_str(), tag.c_str());
}
DBUG("Found process variable \'%s\' for config tag <%s>.",
variable->getName().c_str(), tag.c_str());

This comment has been minimized.

@endJunction

endJunction Jan 18, 2019

Member

This part from find_if until here is same as in the
findProcessVariable(std::vector<ProcessVariable> const&, BaseLib::ConfigTree const&, std::string const&)
function. Duplications should be avoided.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

Totally agree with you. Since the utility of the newly-added function has covered that of function findProcessVariable(std::vector const&, BaseLib::ConfigTree const&, std::string const&), we may consider to drop this function and use the new function in all the processes afterwards.

#include "ProcessLib/Utils/InitShapeMatrices.h"

namespace ProcessLib
{
class ProcessVariable;

This comment has been minimized.

@TomFischer

TomFischer Jan 18, 2019

Member

Since you include the file 'ProcessVariable.h' in line 28 I think the forward declaration isn't needed.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

Dropped the unneeded forward declaration.

@@ -85,14 +97,20 @@ class LocalAssemblerData : public ComponentTransportLocalAssemblerInterface
std::size_t const local_matrix_size,
bool is_axially_symmetric,
unsigned const integration_order,
ComponentTransportProcessData const& process_data)
ComponentTransportProcessData const& process_data,
std::vector<std::vector<std::reference_wrapper<ProcessVariable>>> const&

This comment has been minimized.

@TomFischer

TomFischer Jan 18, 2019

Member

I think it is possible to pass
std::vector<std::reference_wrapper<ProcessVariable>> const& process_variables
instead of std::vector<std::vector<std::reference_wrapper<ProcessVariable>>>.
Then you could write later on (for instance in line 113):
_process_variables.size().

This comment has been minimized.

@renchao-lu

renchao-lu Jan 18, 2019

Author Member

_process_variables is derived from the base class of Process. If using std::vector<std::reference_wrapper> instead, we may encounter problems when adopting staggered scheme.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 22, 2019

Author Member

Dropped the two-dimensional vector.

* | c2p | 0 | c2c2| 0 |
* |-----|-----|-----|-----|
* | c3p | 0 | 0 | c3c3|
*/

This comment has been minimized.

@TomFischer

TomFischer Jan 18, 2019

Member

Nice documentation 🍰

auto local_C = Eigen::Map<const NodalVectorType>(
&local_x[concentration_index], concentration_size);

// Prevent duplicate computation in loops

This comment has been minimized.

@TomFischer

TomFischer Jan 18, 2019

Member

What does this mean? As far as I can see the pure pressure parts, i.e., Mpp and Kpp are recomputed for each component. Is this avoidable?

This comment has been minimized.

@renchao-lu

renchao-lu Jan 19, 2019

Author Member

Fixed. Block matrices Mpp, Kpp, and bp are computed in the first loop over components.

@renchao-lu renchao-lu force-pushed the renchao-lu:MultiComponentTransportProcess branch from 440b085 to 9f92a7f Jan 19, 2019

}
}

void assembleBlockMatrices(

This comment has been minimized.

@TomFischer

TomFischer Jan 21, 2019

Member

Please, clang format the complete function signature.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 21, 2019

Author Member

I rerun clang-format. There is no changes in function signature, though.

This comment has been minimized.

@TomFischer

TomFischer Jan 22, 2019

Member

Seems to be a bug in clang format. If you put all the code of the function signature in one line and rerun clang format the result will change.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 22, 2019

Author Member

It works! I get the right clang-format outputs.


if (it != collected_process_variables.end())
OGS_FATAL(
"Number of components for process variable \"%s\" should be 1 "

This comment has been minimized.

@TomFischer

TomFischer Jan 21, 2019

Member

Please, use ' instead of \" for consistency with other message.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 21, 2019

Author Member

Fixed.


// Use the fluid density model to compute the density
// TODO (renchao): concentration of which component as the argument
// for calculation of fluid density
vars[static_cast<int>(
MaterialLib::Fluid::PropertyVariableType::C)] = C_int_pt;
vars[static_cast<int>(
MaterialLib::Fluid::PropertyVariableType::p)] = p_int_pt;
auto const density = _process_data.fluid_properties->getValue(

This comment has been minimized.

@TomFischer

TomFischer Jan 21, 2019

Member

Here, the density calculation depends only on one of the concentrations. Is this correct? Sorry, I read the comment to late.

@@ -43,17 +44,66 @@ std::vector<std::reference_wrapper<ProcessVariable>> findProcessVariables(
std::vector<ProcessVariable> const& variables,
BaseLib::ConfigTree const& pv_config,
std::initializer_list<std::string>
tag_names)
tags)

This comment has been minimized.

@TomFischer

TomFischer Jan 21, 2019

Member

Please, clang format.

This comment has been minimized.

@endJunction

endJunction Jan 21, 2019

Member

I checked that too, it is the way how clang-format outputs it.

This comment has been minimized.

@renchao-lu

renchao-lu Jan 22, 2019

Author Member

Moving the function signature into one line and then running clang-format will produce what you expect. Once I move into next step to make a commit, the pre-commit hook will tell me that the staged content is not formatted correctly. I would suggest to keep it so far.

@@ -40,7 +40,12 @@ std::vector<std::reference_wrapper<ProcessVariable>> findProcessVariables(
std::vector<ProcessVariable> const& variables,
BaseLib::ConfigTree const& process_config,
std::initializer_list<std::string>
tag_names);
tags);

This comment has been minimized.

@TomFischer

TomFischer Jan 21, 2019

Member

Please, clang format.

@@ -201,16 +216,16 @@
<linear_solvers>
<linear_solver>
<name>general_linear_solver</name>
<lis>-i bicgstab -p ilut -tol 1e-8 -maxiter 20000</lis>
<lis>-i bicgstab -p ilut -tol 1e-16 -maxiter 20000</lis>

This comment has been minimized.

@TomFischer

TomFischer Jan 21, 2019

Member

Why did you choose such a strong tolerance criterion (1e-16)?

This comment has been minimized.

@renchao-lu

renchao-lu Jan 21, 2019

Author Member

I have no idea on the setting of linear solver... @endJunction

@TomFischer

This comment has been minimized.

Copy link
Member

TomFischer commented Jan 21, 2019

Please remove 'Tests/Data/Parabolic/ComponentTransport/SimpleSynthetics/Runtime_Record_Sheet.pdf' and 'Tests/Data/Parabolic/ComponentTransport/SimpleSynthetics/Runtime_Record_Sheet.pdf.x'.

vdbc_pcs_0_ts_9990_t_90000.000000_expected.vtu vdbc_pcs_0_ts_9990_t_90000.000000.vtu concentration Si 1e-5 1e-4
vdbc_pcs_0_ts_15990_t_150000.000000_expected.vtu vdbc_pcs_0_ts_15990_t_150000.000000.vtu concentration Si 1e-5 1e-4
vdbc_pcs_0_ts_21990_t_210000.000000_expected.vtu vdbc_pcs_0_ts_21990_t_210000.000000.vtu concentration Si 1e-5 1e-4
vdbc_pcs_0_ts_25990_t_250000.000000_expected.vtu vdbc_pcs_0_ts_25990_t_250000.000000.vtu concentration Si 1e-5 1e-4
)

This comment has been minimized.

@TomFischer

TomFischer Jan 21, 2019

Member

Are the results of the new test cases (ConcentrationDiffusionOnly_10Comp.prj and ConcentrationDiffusionOnly_30Comp.prj) compared with expected results somewhere in Tests.cmake?

This comment has been minimized.

@renchao-lu

renchao-lu Jan 21, 2019

Author Member

Supplemented a more simple benchmark for testing which includes 3 components.

@TomFischer
Copy link
Member

TomFischer left a comment

Some things to discuss and fix. When tests are green and the fixes are done:

@renchao-lu

This comment has been minimized.

Copy link
Member Author

renchao-lu commented Jan 21, 2019

Two pdf files have been removed. @TomFischer

@renchao-lu renchao-lu force-pushed the renchao-lu:MultiComponentTransportProcess branch 3 times, most recently from 1f236ca to 48a1eae Jan 21, 2019

@@ -50,7 +50,8 @@ void ComponentTransportProcess::initializeConcreteProcess(
ProcessLib::createLocalAssemblers<LocalAssemblerData>(
mesh.getDimension(), mesh.getElements(), dof_table,
pv.getShapeFunctionOrder(), _local_assemblers,
mesh.isAxiallySymmetric(), integration_order, _process_data);
mesh.isAxiallySymmetric(), integration_order, _process_data,
_process_variables[0]);

This comment has been minimized.

@endJunction

endJunction Jan 22, 2019

Member
Suggested change
_process_variables[0]);
_process_variables[process_id]);

This comment has been minimized.

@renchao-lu

renchao-lu Jan 22, 2019

Author Member

Changed.

@renchao-lu renchao-lu force-pushed the renchao-lu:MultiComponentTransportProcess branch from 656f3f1 to ad2460e Jan 22, 2019

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Jan 22, 2019

@renchao-lu When you finish your fixes, please tell, so we can finish the review. As I see now, there are still open questions.

@endJunction endJunction force-pushed the renchao-lu:MultiComponentTransportProcess branch from ad2460e to 5421278 Jan 22, 2019

@renchao-lu renchao-lu force-pushed the renchao-lu:MultiComponentTransportProcess branch from 5421278 to 5f8250a Jan 22, 2019

@renchao-lu renchao-lu force-pushed the renchao-lu:MultiComponentTransportProcess branch from 5f8250a to d7a2f41 Jan 22, 2019

@TomFischer TomFischer merged commit 5d7e608 into ufz:master Jan 23, 2019

2 of 3 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details

@renchao-lu renchao-lu deleted the renchao-lu:MultiComponentTransportProcess branch Jan 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.