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

hc process revision #2200

Merged
merged 8 commits into from Sep 18, 2018

Conversation

Projects
None yet
3 participants
@jbathmann
Copy link
Contributor

jbathmann commented Sep 3, 2018

Revision of HC process in order to account for non constant density model as derived in the pdf attached.
A benchmark in order to compare a analytical steady state solution of the corresponding constant density model to the numerical results is ready. Here I'd like to check the Neumann boundary conditions for component concentration, since they were not working properly beforehand.
I am not sure how to add and explain this benchmark.
HC-Process.pdf

@TomFischer

This comment has been minimized.

Copy link
Member

TomFischer commented Sep 4, 2018

Hi Jasper, congratulation to your first PR!

@@ -11,7 +11,7 @@

#include <Eigen/Dense>
#include <vector>

#include <iostream>

This comment has been minimized.

@TomFischer

TomFischer Sep 4, 2018

Member

Is this needed somewhere?

This comment has been minimized.

@jbathmann

jbathmann Sep 4, 2018

Author Contributor

Nope.
Just removed it. Is it possible to add this change to the pull request?

@@ -33,13 +33,22 @@ template <typename NodalRowVectorType, typename GlobalDimNodalMatrixType>
struct IntegrationPointData final
{
IntegrationPointData(NodalRowVectorType const& N_,
GlobalDimNodalMatrixType const& dNdx_,
GlobalDimNodalMatrixType const& dNdx_,

This comment has been minimized.

@TomFischer

TomFischer Sep 4, 2018

Member

Is it planed to use different shape matrices for the p process and the C process? If it is always the same we could save a lot of memory when the implementation would use only one set of shape matrices.

This comment has been minimized.

@jbathmann

jbathmann Sep 4, 2018

Author Contributor

Yep, finally understood why one set of shape functions is sufficient. Thanks!

@TomFischer

This comment has been minimized.

Copy link
Member

TomFischer commented Sep 4, 2018

We can add the benchmark together if you want.

@jbathmann jbathmann force-pushed the jbathmann:new_hc_process branch from 559efc5 to f4c2dd8 Sep 4, 2018

@jbathmann

This comment has been minimized.

Copy link
Contributor Author

jbathmann commented Sep 4, 2018

Sorry for the mess within the commits. I am still a bit confused in how to properly use git... ;)

@TomFischer TomFischer added the WIP 🏗 label Sep 5, 2018

@jbathmann jbathmann force-pushed the jbathmann:new_hc_process branch from 57ef12e to a6b8f86 Sep 7, 2018


auto const p_nodal_values = Eigen::Map<const NodalVectorType>(
&local_x[ShapeFunction::NPOINTS], ShapeFunction::NPOINTS);
auto const num_nodes = ShapeFunction::NPOINTS;

This comment has been minimized.

@endJunction

endJunction Sep 7, 2018

Member

@TomFischer Seems there is no difference (in the optimized assembly) whether to use NPOINTS or num_nodes for the Eigen::Map creation.

@jbathmann jbathmann force-pushed the jbathmann:new_hc_process branch from a6b8f86 to defbe10 Sep 13, 2018

@jbathmann

This comment has been minimized.

Copy link
Contributor Author

jbathmann commented Sep 13, 2018

Some of the benchmarks have been updated. Additionally a new density model \rho(C,p) = \rho_ref(1+ A_p * p + A_C * C) is added in order to test the new HC process, where the storage parameter has been replaced by:
S= \frac{\phi}{\rho_ref} * \frac{\partial \rho}{\partial p}
Some observations on the benchmarks are documented in the file attached.
20180912HCTests.pdf

@@ -0,0 +1 @@
---

This comment has been minimized.

@endJunction

endJunction Sep 13, 2018

Member

spurious file...

# EXECUTABLE_ARGS goswami_input.prj
# WRAPPER time
# TESTER vtkdiff
# REQUIREMENTS NOT OGS_USE_MPI

This comment has been minimized.

@endJunction

endJunction Sep 13, 2018

Member

Adding a non-existing value to REQUIREMENTS should also disable the test:

REQUIREMENTS NOT OGS_USE_MPI AND DISABLE_GOSWAMI_UNTIL_CONSTRAINED_NEUMANN_BC
@@ -37,7 +37,7 @@
<type>Constant</type>
<porosity_parameter>constant_porosity_parameter</porosity_parameter>
</porosity>
<storage>
<storage>

This comment has been minimized.

@endJunction

endJunction Sep 13, 2018

Member

unneeded white space changes...

@endJunction
Copy link
Member

endJunction left a comment

Looks good.

I'd try to avoid whitespace changes, if possible.

@endJunction endJunction added please review and removed WIP 🏗 labels Sep 13, 2018

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Sep 13, 2018

There are new tags in the project files; they must be documented.
The pdf is very nice and should be included in the benchmarks site.

I can show you the corresponding procedures to include the changes.

@TomFischer

This comment has been minimized.

Copy link
Member

TomFischer commented Sep 14, 2018

Very nice summary of changes in benchmark results!

@@ -0,0 +1,31 @@
<?xml version="1.0"?>

This comment has been minimized.

@TomFischer

TomFischer Sep 14, 2018

Member

Did you add this file for comparison reasons?

This comment has been minimized.

@jbathmann

jbathmann Sep 14, 2018

Author Contributor

Nope. Was an "accident"...

@TomFischer
Copy link
Member

TomFischer left a comment

Minor things to do, after that:

@jbathmann jbathmann force-pushed the jbathmann:new_hc_process branch from 3b697cd to 93e1690 Sep 14, 2018

@jbathmann

This comment has been minimized.

Copy link
Contributor Author

jbathmann commented Sep 14, 2018

Thank you for your comments.

@endJunction I am happy to document the new tags for project files when I am back to Leipzig on Monday.

@jbathmann jbathmann force-pushed the jbathmann:new_hc_process branch 2 times, most recently from aa4f374 to 47970e8 Sep 17, 2018

jbathmann added some commits Aug 31, 2018

[HC] first version of new HC process runs
[HC] first annotations included

[HC] reduction to one shape matrix N

[HC] clang-format reformatting of branch
[T] HC Tests updated
-elder benchmark updated

-heterogenous HC benchmark reference files updated

-goswami benchmark temporarly removed from HC-tests

-SimplySynthetics: ConcentrationAndDiffusionOnly needed small changes in .prj file

-SimpleSynthetics: Project file adapted

-SimpleSynthetics :DiffusionAndStorageAndAdvection modified. Variable density model (p and c dependant) added, non boussinesq effects observed, consistency check,
		update of reference files

-SimpleSynthetics: DiffusionAndStorageAndGravityAndDispersionHalf- A new project has been set up, since the old setup was not converging anymore due to too large
		differences in density leading to problems in non boussinesq treatment.

-SimpleSynthetics: DiffusionAndStorageAndAdvectionAndDispersion- reference files updated. _max iter value in project file sligthly increased.

-SimpleSynthetics: DiffusionAndStorageAndAdvectionAndDecay- reference files updated. density model in prj file changed in order to account for storage.

-SimpleSynthetics: DiffusionAndStorageAndAdvectionAndDispersionHalf- reference files updated. density model in prj file changed in order to account for storage.

jbathmann added some commits Sep 14, 2018

[D] Benchmark test results and revised HC Process derivation included…
… in the web documentation.

Note: The differences for the comparison of the comparison between ogs5 and ogs6 arose from an abs(..) which I added compared to Marc's approach. If I do the same for the calculation, there is no difference.

@jbathmann jbathmann force-pushed the jbathmann:new_hc_process branch from f864203 to effd31a Sep 18, 2018

@endJunction endJunction merged commit a8394b8 into ufz:master Sep 18, 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
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.