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

Fixed the calculation of the Darcy velocity in staggered TH #2127

Merged
merged 4 commits into from Jun 1, 2018

Conversation

Projects
None yet
3 participants
@wenqing
Copy link
Member

wenqing commented May 18, 2018

Follow up PR #2124, this PR fixes a bug in the calculation of the Darcy velocity in the staggered TH. The bug is caused by passing wrong parameters to the arguments of the velocity calculation function.

To fix the bug and also make the source code more readable, process indices are introduced in the local assembler of the staggered scheme for HT.

Due to the partitioning of nodal/element properties being available now, the number of parallel compute cores for the two related benchmarks are increased to three from one.

@@ -140,8 +143,8 @@ void HTProcess::assembleWithJacobianConcreteProcess(
// Call global assembler for each local assembly item.
GlobalExecutor::executeMemberDereferenced(
_global_assembler, &VectorMatrixAssembler::assembleWithJacobian,
_local_assemblers, dof_tables, t, x, xdot, dxdot_dx,
dx_dx, M, K, b, Jac, _coupled_solutions);
_local_assemblers, dof_tables, t, x, xdot, dxdot_dx, dx_dx, M, K, b,

This comment has been minimized.

@wenqing

wenqing May 18, 2018

Author Member

Changes due to clang-format

@@ -181,7 +184,7 @@ void HTProcess::setCoupledTermForTheStaggeredSchemeToLocalAssemblers()
}

std::tuple<NumLib::LocalToGlobalIndexMap*, bool>
HTProcess::getDOFTableForExtrapolatorData() const
HTProcess::getDOFTableForExtrapolatorData() const

This comment has been minimized.

@wenqing

wenqing May 18, 2018

Author Member

Change due to clang-format

std::move(all_mesh_subsets_single_component),
// by location order is needed for output
NumLib::ComponentOrder::BY_LOCATION), manage_storage);
std::move(all_mesh_subsets_single_component),

This comment has been minimized.

@wenqing

wenqing May 18, 2018

Author Member

Change due to clang-format

local_M.noalias() += w * (porosity * dfluid_density_dp / fluid_density +
specific_storage) *
N.transpose() * N;
local_M.noalias() +=

This comment has been minimized.

@wenqing

wenqing May 18, 2018

Author Member

Change due to clang-format

@TomFischer

This comment has been minimized.

Copy link
Member

TomFischer commented May 25, 2018

Thanks @wenqing for fixing the darcy velocity calculation in the HT staggered implementation. In PR #2124 some simple benchmarks were introduced (see Tests/Data/Parabolic/HT/SimpleSynthetics/) for the monolithic implementation. Please, could you add similar benchmarks for the staggered scheme?

@TomFischer
Copy link
Member

TomFischer left a comment

As mentioned, some additional simple benchmarks would be nice.

@chleh

chleh approved these changes May 28, 2018

Copy link
Collaborator

chleh left a comment

You changed the tolerances of the ctests substantially. Is that necessary?

@wenqing

This comment has been minimized.

Copy link
Member Author

wenqing commented May 29, 2018

@TomFischer I will add that benchmark

@wenqing

This comment has been minimized.

Copy link
Member Author

wenqing commented May 29, 2018

@chleh That benchmark, which sounds like a sensitive analysis, gets slightly difference between the reference result and the result by the present master branch by using either the monolithic scheme or the staggered scheme. Since that benchmark is marked as LARGE, it is not checked on jenkins. For which change in the code for the slight discrepancy on results is still under investigating . The other benchmarks are OK.

@wenqing wenqing force-pushed the wenqing:fixing branch from 785da9c to fd6ec59 May 31, 2018

@TomFischer TomFischer merged commit e68ed08 into ufz:master Jun 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
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.