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

Fix volumetric source terms #2241

Merged
merged 3 commits into from Oct 23, 2018

Conversation

Projects
None yet
3 participants
@TomFischer
Copy link
Member

TomFischer commented Oct 12, 2018

  • Fix wrong sign
  • Change tests according to this fix and update the documentation
  • Added non-constant source term test similar to the python non-constant source term test
@@ -105,7 +105,7 @@ class VolumetricSourceTermLocalAssembler final
pos.setIntegrationPoint(ip);
auto const st_val = _volumetric_source_term(t, pos)[0];

_local_rhs.noalias() +=

This comment has been minimized.

@chleh

chleh Oct 15, 2018

Collaborator

Is it really wise to change that sign? IMHO the source term should be added to the r.h.s. of the equation system. This is what you'd naively expect. Or am I missing something?
As far as I can see the test of this feature is only the groundwater flow process. If there is a problem with the sign convention only for that process, maybe sign of the assembly of GWFlow should be reversed.
Since the sign is not unique, a convention should probably be formulated. I'd suggest: A positive source term for the heat transport process should lead to rising temperature in an isolated system.

This comment has been minimized.

@TomFischer

TomFischer Oct 23, 2018

Author Member

You are right. I used the wrong equation for the derivation of the analytical solution. I reversed the changes and updated the documentation.

@TomFischer TomFischer force-pushed the TomFischer:FixVolumetricSourceTerms branch from 495968f to 2150a04 Oct 22, 2018

@chleh

chleh approved these changes Oct 23, 2018

Copy link
Collaborator

chleh left a comment

Only a minor comment

TESTER vtkdiff
REQUIREMENTS NOT (OGS_USE_LIS OR OGS_USE_MPI)
DIFF_DATA
square_1x1_quad_1e3_volumetricsourcetermdataarray.vtu square_1e3_volumetricsourcetermdataarray_pcs_0_ts_1_t_1.000000.vtu analytical_solution pressure 2e-2 1e-16

This comment has been minimized.

@chleh

chleh Oct 23, 2018

Collaborator

This compares to an analytical solution? (Maybe sin(x)*sin(y))? Otherwise the absolute tolerance would be pretty bad. Maybe you could mention the fact that you compare to an analytical solution, and also to which one.

@TomFischer TomFischer force-pushed the TomFischer:FixVolumetricSourceTerms branch from 2150a04 to 6483360 Oct 23, 2018

@TomFischer TomFischer merged commit d488d5c into ufz:master Oct 23, 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:FixVolumetricSourceTerms branch Oct 23, 2018

@TomFischer

This comment has been minimized.

Copy link
Member Author

TomFischer commented Oct 23, 2018

Thanks @chleh and @endJunction for the review!

TESTER vtkdiff
REQUIREMENTS NOT (OGS_USE_LIS OR OGS_USE_MPI)
# the analytical solution is: sin(2*Pi*x-Pi/2)*sin(2*Pi*y-Pi/2)
# the source term in the data array was set to: -2*(2*Pi)^2 * sin(2*Pi*x-Pi/2)*sin(2*Pi*y-Pi/2)

This comment has been minimized.

@chleh

chleh Oct 29, 2018

Collaborator

Thanks for adding this comment. Could be helpful if somebody has to touch this test case in the future.

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.