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

Make local source term definition possible #2261

Merged
merged 8 commits into from Nov 14, 2018

Conversation

Projects
None yet
4 participants
@TomFischer
Copy link
Member

TomFischer commented Nov 9, 2018

This PR restricts the definition of the source term to a sub-domain of the entire simulation domain.

@@ -34,8 +34,8 @@ namespace ProcessLib
{
std::unique_ptr<SourceTerm> createNodalSourceTerm(
BaseLib::ConfigTree const& config, MeshLib::Mesh const& st_mesh,
const NumLib::LocalToGlobalIndexMap& dof_table, std::size_t mesh_id,
const int variable_id, const int component_id,
std::unique_ptr<NumLib::LocalToGlobalIndexMap>&& dof_table,

This comment has been minimized.

@endJunction

endJunction Nov 9, 2018

Member

[discussion] I know, we have it in several other places done the same way, but I think that && is not needed for a unique_ptr because it is move only anyway. Passing it by value is sufficient.

This comment has been minimized.

@TomFischer

TomFischer Nov 9, 2018

Author Member

Yes, this works. I changed it in nodal, volumetric and the Python source term implementations and rebased.

This comment has been minimized.

@chleh

chleh Nov 12, 2018

Collaborator

IMHO the rule "Add && to a type if you don't allow copies." is simpler "Add && to a type if you don't allow copies. But if the type is move-only, you can skip the &&."

@TomFischer TomFischer force-pushed the TomFischer:MakeLocalSourceTermDefinitionPossible branch from 6ae8617 to 1d17b54 Nov 9, 2018

@chleh
Copy link
Collaborator

chleh left a comment

Code looks good. Waiting for approval if this implementation works in general.

*config.component_id);
}

if (type == "Python")
{
#ifdef OGS_USE_PYTHON
// Create local DOF table from the source term mesh subset for the
// given variable and component id.

This comment has been minimized.

@chleh

chleh Nov 12, 2018

Collaborator

In Python probably for all vars/comps.
Probably, these comments can all be removed. They just describe the already very descriptive line of code that follows them.

@@ -2,6 +2,7 @@
<OpenGeoSysProject>
<meshes>
<mesh>square_1x1_quad_1e2.vtu</mesh>
<mesh>square_1x1_quad_1e2_volumetric_source_term.vtu</mesh>

This comment has been minimized.

@chleh

chleh Nov 12, 2018

Collaborator

This is still the entire mesh, isn't it? Is it special in any way? E.g., does it have an element property like original_element_ids? Btw. I didn't find code changes related to mapping d.o.f.s on the source term mesh to d.o.f.s on the whole domain. Does the current code work if the source term is applied to parts of the mesh only (in particular if the ST subset does not start from element ID 0)?

This comment has been minimized.

@TomFischer

TomFischer Nov 13, 2018

Author Member

Created test for strict subset.

This comment has been minimized.

@chleh

chleh Nov 14, 2018

Collaborator

OK, thanks.

{
OGS_FATAL(
"Variable id or component id too high. Actual values: (%d, "
"%d), maximum values: (%d, %d).",

This comment has been minimized.

@chleh

chleh Nov 12, 2018

Collaborator

Btw.: the "maximum" is off by one. What is printed is in C style one past maximum. This is confusing. It's wrong everywhere, and I think I made it wrong for the first time. 😄

This comment has been minimized.

@TomFischer

TomFischer Nov 13, 2018

Author Member

In dof_table_bulk.getNumberOfVariables() the size - 1 is returned, so the message for the number of variables should be consistent. dof_table_bulk.getNumberOfVariableComponents(variable_id) returns a difference where the off by one should be canceled out.

This comment has been minimized.

@chleh

chleh Nov 13, 2018

Collaborator

OK, maybe it's not easy to see from the source code. A short test (HT process, wrong Dirichlet BC):
error: Variable id or component id too high. Actual values: (0, 1), maximum values: (2, 1). at DirichletBoundaryCondition.h, line 46
Confusing, isn't it? As I said, it's not your fault...

@TomFischer TomFischer force-pushed the TomFischer:MakeLocalSourceTermDefinitionPossible branch 2 times, most recently from 5861210 to c8f23c1 Nov 13, 2018

@wenqing
Copy link
Member

wenqing left a comment

Looks good.

DIFF_DATA
line_1_line_1e2_pcs_0_ts_500_t_39062500.000000_reference.vtu line_1_line_1e2_pcs_0_ts_500_t_39062500.000000.vtu temperature temperature 1e-11 0.0
REQUIREMENTS NOT OGS_USE_MPI
)

This comment has been minimized.

@wenqing

wenqing Nov 13, 2018

Member

Could you please add a test for PETSc as

# test the source term on a subdomain with the PETSc embedded executable file
AddTest(
    NAME 1D_HeatConduction_dirichlet_SourceTerm_PETSc
    PATH Parabolic/T/1D_dirichlet_source-term
    EXECUTABLE_ARGS line_1_line_1e2_source_term.prj
    WRAPPER mpirun
    WRAPPER_ARGS -np 1
    TESTER vtkdiff
    REQUIREMENTS OGS_USE_MPI
    DIFF_DATA
    line_1_line_1e2_pcs_0_ts_500_t_39062500.000000_reference.vtu line_1_line_1e2_pcs_0_ts_500_t_39062500.000000_0.vtu temperature temperature 1e-11 0.0
)

This comment has been minimized.

@TomFischer

TomFischer Nov 14, 2018

Author Member

Test for mpi is added.

@endJunction
Copy link
Member

endJunction left a comment

PETSc test; TODO for (%d, %d) output;

@TomFischer TomFischer force-pushed the TomFischer:MakeLocalSourceTermDefinitionPossible branch from c8f23c1 to 49fd5b8 Nov 14, 2018

@TomFischer

This comment has been minimized.

Copy link
Member Author

TomFischer commented Nov 14, 2018

Created issue #2266 for 'TODO for (%d, %d)'.

@endJunction endJunction merged commit 970229f into ufz:master Nov 14, 2018

3 checks passed

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

@TomFischer TomFischer deleted the TomFischer:MakeLocalSourceTermDefinitionPossible branch Nov 14, 2018

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.